http.client.HTTPResponse.read(-1) handled incorrectly #112064

smason · 2023-11-14T17:03:08Z

Bug report

Bug description:

http.client.HTTPResponse doesn't handle negative reads the same as other readers, for example the following code will hang for a significant amount of time:

con = http.client.HTTPConnection("httpbin.org")
con.request("GET", "/get")
resp = con.getresponse()
# "connection: close" doesn't trigger this
assert resp.headers["connection"] == "keep-alive"
while chunk := resp.read(-1):
    print(chunk)

The negative parameter is passed onto the underlying socket which will cause it to try and read to the end-of-stream. For keep-alive connections this just blocks until the connection is closed by the server due to inactivity.

I think this is a bug with not checking for negative amt values in:

cpython/Lib/http/client.py

Lines 469 to 471 in 24216d0

    
           if self.length is not None and amt > self.length: 
        
               # clip the read to the "end of response" 
        
               amt = self.length

Changing the call to read1 causes the above to display the response promptly as I'd expect. This is due to it correctly checking for negative sizes.

cpython/Lib/http/client.py

Lines 654 to 655 in 24216d0

    
           if self.length is not None and (n < 0 or n > self.length): 
        
               n = self.length

Note that in earlier Python versions, e.g. 3.9, the above fails with ValueError: negative count which seems better than timing out, but I think reading to the end of the response makes more sense.

CPython versions tested on:

3.11, 3.12

Operating systems tested on:

Linux

Linked PRs

The text was updated successfully, but these errors were encountered:

illia-v · 2023-11-14T22:34:57Z

@smason thanks for opening the issue!

I'll bring a bit more context from the urllib3/urllib3#3186 discussion here.
http.client.HTTPResponse is said to implement io.BufferedIOBase since Python 3.5. Then io.BufferedIOBase.read specifies that if its "argument is omitted, None, or negative, data is read and returned until EOF is reached".

This together leads to an assumption that the read method should treat any negative number the same as it treats None in the whole following block:

cpython/Lib/http/client.py

Lines 468 to 494 in 24216d0

    
           if amt is not None: 
        
               if self.length is not None and amt > self.length: 
        
                   # clip the read to the "end of response" 
        
                   amt = self.length 
        
               s = self.fp.read(amt) 
        
               if not s and amt: 
        
                   # Ideally, we would raise IncompleteRead if the content-length 
        
                   # wasn't satisfied, but it might break compatibility. 
        
                   self._close_conn() 
        
               elif self.length is not None: 
        
                   self.length -= len(s) 
        
                   if not self.length: 
        
                       self._close_conn() 
        
               return s 
        
           else: 
        
               # Amount is not given (unbounded read) so we must check self.length 
        
               if self.length is None: 
        
                   s = self.fp.read() 
        
               else: 
        
                   try: 
        
                       s = self._safe_read(self.length) 
        
                   except IncompleteRead: 
        
                       self._close_conn() 
        
                       raise 
        
                   self.length = 0 
        
               self._close_conn()        # we read everything 
        
               return s

ljmc-github · 2023-11-17T17:52:30Z

If no one is working on a PR, I’m happy to take this.

smason · 2023-11-17T18:38:06Z

I was waiting from somebody official to say whether this was a real issue before making a PR...

I;ve also noticed that http.client.HTTPResponse doesn't have read1 documented and was thinking of putting that in as well.

…onse.read()` (#128270) The parameter `amt` of `HTTPResponse.read()`, which could be a negative integer, has not been handled before and led to waiting for the connection to close for `keep-alive connections`. Now, this has been fixed, and passing negative values to `HTTPResponse().read()` works the same as passing `None` value.

…TPResponse.read()` (pythonGH-128270) The parameter `amt` of `HTTPResponse.read()`, which could be a negative integer, has not been handled before and led to waiting for the connection to close for `keep-alive connections`. Now, this has been fixed, and passing negative values to `HTTPResponse().read()` works the same as passing `None` value. (cherry picked from commit 4d0d24f) Co-authored-by: Yury Manushkin <manushkin@gmail.com>

…TTPResponse.read()` (GH-128270) (#129396) gh-112064: Fix incorrect handling of negative read sizes in `HTTPResponse.read()` (GH-128270) The parameter `amt` of `HTTPResponse.read()`, which could be a negative integer, has not been handled before and led to waiting for the connection to close for `keep-alive connections`. Now, this has been fixed, and passing negative values to `HTTPResponse().read()` works the same as passing `None` value. (cherry picked from commit 4d0d24f) Co-authored-by: Yury Manushkin <manushkin@gmail.com>

…TTPResponse.read()` (GH-128270) (#129395) gh-112064: Fix incorrect handling of negative read sizes in `HTTPResponse.read()` (GH-128270) The parameter `amt` of `HTTPResponse.read()`, which could be a negative integer, has not been handled before and led to waiting for the connection to close for `keep-alive connections`. Now, this has been fixed, and passing negative values to `HTTPResponse().read()` works the same as passing `None` value. (cherry picked from commit 4d0d24f) Co-authored-by: Yury Manushkin <manushkin@gmail.com>

smason added the type-bug An unexpected behavior, bug, or error label Nov 14, 2023

Eclips4 added the stdlib Python modules in the Lib dir label Nov 14, 2023

smason mentioned this issue Nov 14, 2023

Support read1() method in HTTPResponse urllib3/urllib3#3186

Merged

illia-v mentioned this issue Nov 29, 2023

make HTTPResponse.stream use read1 when amt=None urllib3/urllib3#3216

Open

illia-v mentioned this issue Feb 27, 2024

request(..., preload_content=False).read(-1) raises an unexpected Exception urllib3/urllib3#3122

Closed

This was referenced Jan 3, 2025

gh-112064: fix incorrect handling of negative read sizes in HTTPResponse.read() #128270

Merged

Incorrect handling of negative reading sizes in HTTPResponse.read() #128271

Closed

Eclips4 marked this as a duplicate of #128271 Jan 28, 2025

bedevere-app bot mentioned this issue Jan 28, 2025

[3.13] gh-112064: Fix incorrect handling of negative read sizes in HTTPResponse.read() (GH-128270) #129395

Merged

bedevere-app bot mentioned this issue Jan 28, 2025

[3.12] gh-112064: Fix incorrect handling of negative read sizes in HTTPResponse.read() (GH-128270) #129396

Merged

Eclips4 closed this as completed Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http.client.HTTPResponse.read(-1) handled incorrectly #112064

http.client.HTTPResponse.read(-1) handled incorrectly #112064

smason commented Nov 14, 2023 •

edited by bedevere-app bot

Loading

illia-v commented Nov 14, 2023

ljmc-github commented Nov 17, 2023

smason commented Nov 17, 2023

http.client.HTTPResponse.read(-1) handled incorrectly #112064

http.client.HTTPResponse.read(-1) handled incorrectly #112064

Comments

smason commented Nov 14, 2023 • edited by bedevere-app bot Loading

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

illia-v commented Nov 14, 2023

ljmc-github commented Nov 17, 2023

smason commented Nov 17, 2023

smason commented Nov 14, 2023 •

edited by bedevere-app bot

Loading