| msg45398 - (view) |
Author: Bastian Kleineidam (calvin) |
Date: 2004-02-19 23:14 |
| In HTTPResponse._read_chunked the chunk length is not checked to be a valid integer, and a ValueError will be raised in such a case. The attached patch catches ValueError (which should not normally happen, so this try:except: is reasonably fast), and raises IncompleteRead exception instead. I have no test case for this yet, but am trying to construct one :) |
|
|
| msg45399 - (view) |
Author: agwego (agwego) |
Date: 2005-02-28 16:53 |
| Logged In: YES user_id=1228982 I've run into this problem in conjunction with mod_python 3.1.4 (and although the problem is caused in mod_python) my python skills aren't up to the task. Which leaves me with fixing httplib. Although the above patch works when it comes to end of file situations, I think it would be better to return what has been consumed so far and leave it at that. A few lines down there's a comment about consuming trailers, this is the case that is tripping up httplib as far as I can tell. This is happening in Python 2.3.4. --- packages/Python-2.Lib/httplib.py Sun Nov 2 11:51:38 2003 +++ httplib.py Mon Feb 28 11:26:48 2005 @@ -423,7 +423,11 @@ i = line.find(';') if i >= 0: line = line[:i] # strip chunk-extensions - chunk_left = int(line, 16) + try: + chunk_left = int(line, 16) + except ValueError, msg: + self.close() + return value if chunk_left == 0: break if amt is None: |
|
|
| msg45400 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2005-07-18 19:34 |
| Logged In: YES user_id=1188172 Attaching patch which does what agwego said (httplib-chunked.diff). Please review. |
|
|
| msg45401 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2005-07-19 01:07 |
| Logged In: YES user_id=80475 Technically, the patch is fine and it is the way the code should have been written in the first place. Please bring-up on python-dev to determine whether it should be done. I can imagine that a fair amount of existing code has through trial and error discovered the ValueError and chosen to catch that. Changing the exception may unnecessarily break people's code. Sometimes we take that step when it needs to be done, but there should be a good pay-off and, in this case, I don't see it. You may also want to mention it on comp.lang.python to see if anyone cares If the patch goes forward, see if you can mock-up a test that triggers the exception so we have a good unittest. In anycase, this should not be backported (we avoid giving people reasons to not upgrade). |
|
|
| msg45402 - (view) |
Author: Bastian Kleineidam (calvin) |
Date: 2006-02-01 22:16 |
| Logged In: YES user_id=9205 I attached a simple testcase that triggers the bug. IMHO this patch should be applied for Python 2.5. |
|
|
| msg45403 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2007-02-18 08:39 |
| Georg, the patch is fine for 2.6, please apply (for 2.5, I would be cautious because of the behaviour change). |
|
|
| msg45404 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2007-02-19 12:10 |
| Martin: which patch? Mine, which returns what was read so far, or calvin's, which raises IncompleteRead? |
|
|
| msg45405 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2007-02-19 16:05 |
| I meant to suggest that your patch (based on agwego's recommendation) should be accepted. Now that you ask, I'm not so sure anymore: errors should never pass silently. So I now think, that it should raise IncompleteRead, based also on the observation that these methods can *already* raise IncompleteRead, and that this already carries the data that we received. OTOH, IIUC, the error also means that we lost protocol synchronisation with the server, so we should in any case close the connection. So merging these changes is probably the right thing :-) Withdrawing my acceptance again, and hoping that somebody can produce a complete patch (which includes the test case in a style that works with regrtest) |
|
|
| msg45406 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2007-03-13 21:47 |
| See also #1486335. |
|
|
| msg62848 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2008-02-24 00:03 |
| Fixed in r61034, including unittest. |
|
|
| msg68871 - (view) |
Author: Philip Dorrell (pdorrell) |
Date: 2008-06-28 09:37 |
| I have raised a "bug" 125 for boto ( http://code.google.com/p/boto/issues/detail?id=125 ) in relation to this problem, because if httplib.py is not fixed in Python 2.5, then any code calling httplib.py has to work around it by handling a ValueError exception when it occurs. With regard to the fix in Python 2.6, and reading through the httplib.py source, I conclude: * IncompleteRead(value) means something like "we were expecting to read X bytes but actually there aren't any more, and so far we have read *value*". * This meaning does not quite describe the error which this issue is about, which really needs its own separate exception class, e.g. InvalidHttpResponseChunkSize(value, badSize) meaning "we were expecting to read the size of the next chunk, but *badSize* is not a valid number, and so far we have read *value*) |
|
|
| msg76765 - (view) |
Author: John J Lee (jjlee) |
Date: 2008-12-02 18:17 |
| The fix for this doesn't actually close the connection as the comment in the fix claims -- I've raised #4492 to track that. |
|
|