msg69446 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2008-07-08 22:45 |
_multiprocessing.Connection() allows to use any positive (or nul)
number has socket handle. If you use an invalid file descriptor,
poll() method may crash (especially for big positive integer).
Example:
>>> import _multiprocessing
>>> obj = _multiprocessing.Connection(44977608)
>>> obj.poll()
Erreur de segmentation (core dumped)
Fix: use fstat() to make sure that the handle is valid. Attached patch
implements this feature.
Another solution would be to reuse code from Modules/selectmodule.c.
|
msg69448 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2008-07-08 23:54 |
Ooops, there is a typo in my last patch: it's "struct stat statbuf;"
and not "struct stat *statbuf;"! Here is a new version of the patch.
|
msg70425 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2008-07-30 12:06 |
From Victor:
Ok, here is a patch for test_multiprocessing.py.
- TestClosedFile.test_open() verify that Connection() rejects closed file
descriptor
- TestClosedFile.test_operations() verify that Connection() raises IOError
for
operations on closed file
I don't know if Connection() is a socket or a pipe. Both should be tested.
|
msg70426 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *  |
Date: 2008-07-30 12:41 |
I'm quite sure that neither the patch nor the new test make sense on
Windows. A file handle is not a file descriptor!
|
msg73158 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2008-09-13 01:00 |
Without someone offering some windows help, I won't be able to do a patch.
My windows fu is lacking.
|
msg73172 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *  |
Date: 2008-09-13 09:36 |
Note that Windows does not crash in such cases:
>>> import socket, _multiprocessing
>>> obj = _multiprocessing.Connection(44977608)
>>> obj.poll()
IOError: [Errno 10038] An operation was attempted on something that is
not a socket
>>> s = socket.socket()
>>> obj = _multiprocessing.Connection(s.fileno())
>>> obj.poll()
False
>>> s.close()
>>> obj.poll()
IOError: [Errno 10038] An operation was attempted on something that is
not a socket
So some "#ifndef MS_WINDOWS" should be enough...
|
msg73177 - (view) |
Author: Hirokazu Yamamoto (ocean-city) *  |
Date: 2008-09-13 12:02 |
I thought socket handle on BeOS is not file descripter neighter. (I'm
not sure BeOS is still supported or not)
>Another solution would be to reuse code from Modules/selectmodule.c.
You mean this code?
if (v < 0 || v >= FD_SETSIZE) {
PyErr_SetString(PyExc_ValueError,
"filedescriptor out of range in select()");
goto finally;
}
Cygwin's thread is somewhat troublesome, so I'm not sure I can test this
issue on cygwin but, (I'm now building python --with-threads) if clash
happens in conn_poll(_multiprocessing/connection.h)'s FD_SET, I think
this kind of check is needed... (At least safer)
|
msg73192 - (view) |
Author: Hirokazu Yamamoto (ocean-city) *  |
Date: 2008-09-13 15:44 |
I've implemented "another solution". test_open() in
test_multithreading.patch won't pass though.... It'll raise error in
conn.poll() not in constructor.
$ ./dummy.exe b.py
Traceback (most recent call last):
File "b.py", line 6, in <module>
conn.poll()
IOError: [Errno 9] Bad file descriptor
test_operations() will pass.
|
msg80169 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 14:46 |
Attached is a patch+test for this condition, which is not used if we're
running on windows.
|
msg80170 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 14:56 |
Curse you hard-tabs. Here's the new patch w/ fixed comment
|
msg80174 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 15:10 |
Removed raise TestSkip per code review from bpeterson
|
msg80175 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 15:12 |
Committed patch as r68768 to python-trunk
|
msg80177 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2009-01-19 15:22 |
@jnoller: Hey, you removed my patch! My patch used fstat() in
Connection constructor, whereas your just check file descriptor bounds
in the poll() method. And what is the "save" new argument? Is it
related to this issue?
|
msg80178 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 15:24 |
The save was needed for the Py_BLOCK_THREADS call.
|
msg80179 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 15:26 |
Ugh, I didn't mean to chuck your original patch, but it also wasn't
correct for win32
Additionally, if you close the handle from underneath it, it behaves
properly:
>>> obj.poll()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
IOError: [Errno 9] Bad file descriptor
>>>
|
msg80184 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2009-01-19 15:47 |
Why don't you check the file descriptor directly in connection_new()?
conn->handle is read only and so can't be changed before the call to
poll(). So other methods will also be "protected" and the error will
be raised earlier.
|
msg80185 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 15:49 |
That's an enhancement - not a bad idea, I just noticed that this issue is
pretty close to issue http://bugs.python.org/issue3311 as well.
|
msg80186 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2009-01-19 15:56 |
jnoller> issue #3311
Oh, I forgot this issue :-) But the fix doesn't solve #3311, because
it is disabled on Windows and only protect poll() method.
|
msg80187 - (view) |
Author: Jesse Noller (jnoller) *  |
Date: 2009-01-19 16:05 |
Oh, I agree - I think we should update 3311 with the enhancement to move
the check to connection_new
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:36 | admin | set | github: 47571 |
2009-01-19 16:05:33 | jnoller | set | messages:
+ msg80187 |
2009-01-19 15:56:19 | vstinner | set | messages:
+ msg80186 |
2009-01-19 15:49:03 | jnoller | set | messages:
+ msg80185 |
2009-01-19 15:47:13 | vstinner | set | messages:
+ msg80184 |
2009-01-19 15:41:40 | jnoller | set | status: open -> closed resolution: fixed |
2009-01-19 15:26:35 | jnoller | set | messages:
+ msg80179 |
2009-01-19 15:24:15 | jnoller | set | messages:
+ msg80178 |
2009-01-19 15:22:23 | vstinner | set | messages:
+ msg80177 |
2009-01-19 15:12:46 | jnoller | set | messages:
+ msg80175 |
2009-01-19 15:10:50 | jnoller | set | files:
- issue_3321.patch |
2009-01-19 15:10:45 | jnoller | set | files:
+ issue_3321.patch messages:
+ msg80174 |
2009-01-19 14:56:25 | jnoller | set | files:
- issue_3321.patch |
2009-01-19 14:56:18 | jnoller | set | files:
+ issue_3321.patch messages:
+ msg80170 |
2009-01-19 14:46:47 | jnoller | set | files:
- another_solution.patch |
2009-01-19 14:46:43 | jnoller | set | files:
- test_multiprocessing.patch |
2009-01-19 14:46:40 | jnoller | set | files:
- _multiprocessing_connection.patch |
2009-01-19 14:46:22 | jnoller | set | files:
+ issue_3321.patch messages:
+ msg80169 |
2008-09-13 15:44:12 | ocean-city | set | files:
+ another_solution.patch messages:
+ msg73192 |
2008-09-13 12:10:33 | ocean-city | set | keywords:
+ needs review |
2008-09-13 12:02:20 | ocean-city | set | keywords:
- needs review nosy:
+ ocean-city messages:
+ msg73177 |
2008-09-13 09:36:04 | amaury.forgeotdarc | set | messages:
+ msg73172 |
2008-09-13 01:00:41 | jnoller | set | messages:
+ msg73158 |
2008-09-13 00:46:36 | benjamin.peterson | set | priority: high -> critical keywords:
+ needs review |
2008-09-12 23:51:17 | ajaksu2 | set | nosy:
+ ajaksu2 |
2008-07-30 12:41:19 | amaury.forgeotdarc | set | nosy:
+ amaury.forgeotdarc messages:
+ msg70426 |
2008-07-30 12:06:16 | jnoller | set | files:
+ test_multiprocessing.patch messages:
+ msg70425 |
2008-07-19 13:02:56 | georg.brandl | set | priority: high |
2008-07-15 13:34:45 | jnoller | set | nosy:
+ roudkerk |
2008-07-15 13:34:36 | jnoller | set | assignee: jnoller nosy:
+ jnoller |
2008-07-14 13:01:05 | vstinner | set | files:
- _multiprocessing_connection.patch |
2008-07-08 23:54:38 | vstinner | set | files:
+ _multiprocessing_connection.patch messages:
+ msg69448 |
2008-07-08 22:45:18 | vstinner | set | type: crash |
2008-07-08 22:45:09 | vstinner | create | |