msg62602 - (view) |
Author: Rafael Zanella (zanella) |
Date: 2008-02-20 21:20 |
Queue.Queue(), accepts any value as the maxsize, example:
foo = Queue.Queue('j');
l = []; foo = Queue.Queue(l);
...
Shouldn't the value passed be checked on init :
isinstance(maxsize, int) ?
|
msg62603 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-02-20 22:06 |
This is probably a matter of style. For the most part, library code
avoids isinstance() checks and lets the errors surface downstream.
If a check gets added, we should probably also check that the value is
non-negative. The checks should not be backported because it could
break Queue subclasses that rely on being able to pass in a non-int
value (like None or like a dynamic object that allows the maxsize to be
increased during the run).
I'm unclear why this got classified as a security issue rather than just
an RFE for Py2.6.
|
msg62604 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-20 22:15 |
Is there a problem with a dynamic object that changes the maxsize? I
think it might be better to ignore it, and let the client get exceptions
when their maxsize is compared.
|
msg62606 - (view) |
Author: Rafael Zanella (zanella) |
Date: 2008-02-20 23:29 |
Firts: the security type was my error.
The method wich uses the maxsize:
"""
# Check whether the queue is full
def _full(self):
return self.maxsize > 0 and len(self.queue) == self.maxsize
"""
@rhettinger: As per the documentation, negative values result on an
infinite Queue; well that AND will never be fulfilled with a negative
value anyway;
@gutworth: What I mean is that's "awkward", if you put an string for
example, it'll be the size of the string wich will be used on the
__cmp__ and on len(), but that's not explicit, or is it?
Example:
[zan@tails ~]$ python
Python 2.5.1 (r251:54863, Oct 30 2007, 13:54:11)
[GCC 4.1.2 20070925 (Red Hat 4.1.2-33)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C:
... def __init__(self): pass;
...
>>> c = C()
>>> import Queue
>>> a = Queue.Queue(c)
>>> len(c)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: C instance has no attribute '__len__'
>>> a = Queue.Queue(c)
>>> a.put('q')
>>> a.get()
'q'
>>> a.put(1)
>>> a.put(2)
>>> a.put(3)
>>>
|
msg62609 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-21 02:00 |
Rafael, I agree that it's awkward, and I'm not against restricting the
maxsize to just something sane. However, I'm worried this (Raymond's
patch) will be too restrictive by not allowing dynamic changing of
maxsize. (Of course, you could just change the maxsize attribute of the
Queue, but that would require holding the mutex, too.)
|
msg62610 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-02-21 08:39 |
What about requiring maxsize to be convertible to an int?
This would allow dynamic objects, if they define an __int__ method.
I join a patch.
|
msg62620 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-21 12:56 |
I like it.
|
msg62621 - (view) |
Author: Rafael Zanella (zanella) |
Date: 2008-02-21 13:06 |
@gutworth: Since one of the main uses of Queue is with threads, I think
it *really* should acquire the mutex before changing the maxsize;
@amaury.forgeotdarc: Your patch makes the point of allowing the size to
be changed at some other place (e.g.: an attribute of an instance passed
as the maxsize), but as stated above I think (am not an expert) the
mutex really should be held before the maxsize change, another point:
using an instance on your patch a call to Queue.maxsize would return
something like:
"""
Python 2.5.1 (r251:54863, Oct 5 2007, 13:36:32)
[GCC 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C:
... def __init(self): self.n = 1;
... def __int__(self): return int(self.n);
...
>>> c = C()
>>> import Queue
>>> q = Queue.Queue(c)
>>> q.maxsize
<__main__.C instance at 0xb7c341ac>
>>>
"""
wich would force to have a get() to the maxsize attribute;
I have added a diff;
|
msg62624 - (view) |
Author: Rafael Zanella (zanella) |
Date: 2008-02-21 15:34 |
Mine patch doesn't address the "hold the mutex before changing the
maxsize" guess it would then force a get()?
|
msg62629 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-02-21 16:58 |
It's probably best to close this as "won't fix". Each of the patches
limits the module or complicates it a bit. I'm not sure there's even a
real problem here. My preference is to leave this code untouched.
|
msg62630 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-21 16:59 |
Maybe, we should change change the constructor to use self.maxsize =
int(maxsize). Then we can provide a set_maxsize method that will acquire
the mutex and preform the change. This is will restrict the type of
maxsize and allow for easy dynamic changing.
|
msg62632 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-02-21 17:05 |
Recommend closing this. No need to muck-up a clean module to solve a
non-problem.
|
msg62633 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-21 17:07 |
Raymond, are you referring to the int checking, my new method, or both?
|
msg62634 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2008-02-21 17:10 |
Both. There is no issue here worth adding a new method.
|
msg62635 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-21 17:13 |
Ok. I agree that we shouldn't muddy the waters of Queue by checking for
int. (set_maxsize would be unneeded) Go ahead and close.
|
msg62735 - (view) |
Author: Rodrigo Bernardo Pimentel (rbp) |
Date: 2008-02-23 14:57 |
For what it's worth, I do think this is an issue. As it currently
stands, not only does the module silently accept invalid values, but the
mutex issue exists (and is also silently ignored) if an object returning
dynamic values is passed as maxsize. IMHO, the waters are already muddy,
it's just that the mud is blue so everything seems alright :)
I think zanella's patch is the way to go, even if it requires adding a
setter method.
|
msg62768 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-23 16:28 |
Many places in the stdlib accept values which are not valid. I believe
this is because the library trusts you to do the right thing in the name
of performance and cleaner, simpler code. IMO, adding a set_maxsize
method wouldn't be a sin, but Raymond (who is I'm sure wiser than me)
disagrees.
|
msg62800 - (view) |
Author: Rafael Zanella (zanella) |
Date: 2008-02-23 19:39 |
Just to exemplify:
"""
from threading import Thread
import time
import Queue
class C:
def __int__(self):
return 3
#def __del__(self): print "collected..." # won't happen since q holds
a reference to it
c = C()
q = Queue.Queue(c)
# Not dynamic
print "maxsize: ", q.maxsize
# Not full() with instance
print c > 0
print len(q.queue) == q.maxsize
class T(Thread):
def __init__(self, q):
self._q = q
Thread.__init__(self)
def run(self):
#For sme bizarre motive
self._q.maxsize = 5
#Ends up being infinite most of the times
t = T(q)
for i in xrange(1000):
q.put_nowait(i)
if i == 1: # otherwise the "and len(self.queue) == self.maxsize" will fail
t.start()
time.sleep(1)
t.join()
"""
I guess rhettinger is right, there's no issue here, anyone that decides
to change the maxsize afterwards should know what is doing.
The only "possible" problem I'm able to see is someone passing an object
wich has __int__() and expecting it to be used.
|
msg62803 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-02-23 19:56 |
> The only "possible" problem I'm able to see is someone passing an object
wich has __int__() and expecting it to be used.
They should be explicit and say Queue(int(object_with__int__)).
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:31 | admin | set | github: 46402 |
2008-02-23 19:56:58 | benjamin.peterson | set | messages:
+ msg62803 |
2008-02-23 19:39:46 | zanella | set | messages:
+ msg62800 |
2008-02-23 16:28:08 | benjamin.peterson | set | messages:
+ msg62768 |
2008-02-23 14:57:42 | rbp | set | nosy:
+ rbp messages:
+ msg62735 |
2008-02-21 17:18:26 | rhettinger | set | status: open -> closed resolution: wont fix |
2008-02-21 17:13:42 | benjamin.peterson | set | messages:
+ msg62635 |
2008-02-21 17:10:47 | rhettinger | set | messages:
+ msg62634 |
2008-02-21 17:07:10 | benjamin.peterson | set | messages:
+ msg62633 |
2008-02-21 17:05:07 | rhettinger | set | messages:
+ msg62632 |
2008-02-21 16:59:48 | benjamin.peterson | set | messages:
+ msg62630 |
2008-02-21 16:58:53 | rhettinger | set | messages:
+ msg62629 |
2008-02-21 15:34:56 | zanella | set | messages:
+ msg62624 |
2008-02-21 13:06:04 | zanella | set | files:
+ queue_maxsize_3.diff messages:
+ msg62621 |
2008-02-21 12:56:22 | benjamin.peterson | set | messages:
+ msg62620 |
2008-02-21 08:40:00 | amaury.forgeotdarc | set | files:
+ queue_maxsize_2.diff nosy:
+ amaury.forgeotdarc messages:
+ msg62610 |
2008-02-21 02:00:29 | benjamin.peterson | set | messages:
+ msg62609 |
2008-02-21 01:02:53 | rhettinger | set | files:
+ queue_maxsize.diff |
2008-02-20 23:29:27 | zanella | set | type: security -> enhancement messages:
+ msg62606 |
2008-02-20 22:15:09 | benjamin.peterson | set | nosy:
+ benjamin.peterson messages:
+ msg62604 |
2008-02-20 22:06:26 | rhettinger | set | priority: low nosy:
+ rhettinger messages:
+ msg62603 versions:
+ Python 2.6, - Python 2.5, Python 2.4 |
2008-02-20 21:20:40 | zanella | set | messages:
+ msg62602 |
2008-02-20 21:09:20 | zanella | create | |