gh-119451: Fix a potential denial of service in http.client

[serhiy-storchaka]

Reading the whole body of the HTTP response could cause OOM if the Content-Length value is too large even if the server does not send a large amount of data. Now the HTTP client reads large data by chunks, therefore the amount of consumed memory is proportional to the amount of sent data.

• Issue: Out-of-memory when reading a HTTP response with large Content-Length #119451

[gpshead]

I've marked this Draft for now as discussion on this on the security response team list is not complete. (we'll summarize that in a public issue once it has settled)

[encukou]

See #119514 (comment) for results of the PSRT discussion.

[illia-v]

When multiple 1 MB chunks have to be read and joined, io.BytesIO would consume less memory and do the job significantly faster.

Here's a result of a simple benchmark on Python 3.14.0:

Benchmarking with 2 iterations and chunk size 1048576 bytes... Concatenation (+=): Peak Memory = 4.00 MB, Time = 0.0026 seconds BytesIO: Peak Memory = 3.00 MB, Time = 0.0011 seconds Benchmarking with 10 iterations and chunk size 1048576 bytes... Concatenation (+=): Peak Memory = 20.00 MB, Time = 0.0316 seconds BytesIO: Peak Memory = 11.13 MB, Time = 0.0040 seconds 

The benchmarking script

importioimporttimeimporttracemallocdefbenchmark_concatenation(n, chunk_size): tracemalloc.start() start_time=time.time() data=b""foriinrange(n): data+=bytes([i%256]) *chunk_sizeend_time=time.time() current, peak=tracemalloc.get_traced_memory() tracemalloc.stop() returnpeak, end_time-start_timedefbenchmark_bytesio(n, chunk_size): tracemalloc.start() start_time=time.time() buffer=io.BytesIO() foriinrange(n): buffer.write(bytes([i%256]) *chunk_size) # getvalue() creates a copy of the buffer contentresult=buffer.getvalue() end_time=time.time() current, peak=tracemalloc.get_traced_memory() tracemalloc.stop() returnpeak, end_time-start_timedefmain(n): chunk_size=1024*1024# 1 MBprint(f"Benchmarking with {n} iterations and chunk size {chunk_size} bytes...") peak_concat, time_concat=benchmark_concatenation(n, chunk_size) print( f"Concatenation (+=): Peak Memory = {peak_concat/1024/1024:.2f} MB, Time = {time_concat:.4f} seconds" ) peak_bio, time_bio=benchmark_bytesio(n, chunk_size) print( f"BytesIO: Peak Memory = {peak_bio/1024/1024:.2f} MB, Time = {time_bio:.4f} seconds" ) if__name__=="__main__": whileTrue: main(int(input("Enter n: ")))

[serhiy-storchaka]

Your benchmark reads data by chunks with the constant size .

This PR reads it by chunks with geometrically increased size.

[illia-v]

That''s true. Here's the changed script with its result

Benchmarking with 2 iterations and initial chunk size 1048576 bytes... Concatenation (+=): Peak Memory = 6.00 MB, Time = 0.0044 seconds BytesIO: Peak Memory = 5.00 MB, Time = 0.0041 seconds Benchmarking with 10 iterations and initial chunk size 1048576 bytes... Concatenation (+=): Peak Memory = 2046.00 MB, Time = 4.5761 seconds BytesIO: Peak Memory = 1535.00 MB, Time = 1.7045 seconds 

The updated benchmarking script

importioimporttimeimporttracemallocdefbenchmark_concatenation(n, initial_chunk_size): tracemalloc.start() start_time=time.time() data=b""foriinrange(n): data+=bytes([i%256]) * (initial_chunk_size* (2**i)) end_time=time.time() current, peak=tracemalloc.get_traced_memory() tracemalloc.stop() returnpeak, end_time-start_timedefbenchmark_bytesio(n, initial_chunk_size): tracemalloc.start() start_time=time.time() buffer=io.BytesIO() foriinrange(n): buffer.write(bytes([i%256]) * (initial_chunk_size* (2**i))) # getvalue() creates a copy of the buffer contentresult=buffer.getvalue() end_time=time.time() current, peak=tracemalloc.get_traced_memory() tracemalloc.stop() returnpeak, end_time-start_timedefmain(n): chunk_size=1024*1024# 1 MBprint(f"Benchmarking with {n} iterations and initial chunk size {chunk_size} bytes...") peak_concat, time_concat=benchmark_concatenation(n, chunk_size) print( f"Concatenation (+=): Peak Memory = {peak_concat/1024/1024:.2f} MB, Time = {time_concat:.4f} seconds" ) peak_bio, time_bio=benchmark_bytesio(n, chunk_size) print( f"BytesIO: Peak Memory = {peak_bio/1024/1024:.2f} MB, Time = {time_bio:.4f} seconds" ) if__name__=="__main__": whileTrue: main(int(input("Enter n: ")))

[serhiy-storchaka]

I see now why BytesIO consumes 25% less memory. If you add n new bytes to a buffer of size n, in case of the bytes concatenation you need to allocate a new buffer of size 2*n -- total 4*n bytes, but BytesIO can reallocate the original buffer -- total 3*n bytes. It also benefits from CPython-specifix optimization -- BytesIO.getvalue() just returns the internal bytes buffer without allocating a new bytes object for result.

The time difference is perhaps a derivation of this, although it is not important when we read from internet. But the peak memory consumption is important.

[miss-islington-app]

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11, 3.12, 3.13, 3.14.
🐍🍒⛏🤖

[bedevere-app]

GH-142138 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-142139 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-142140 is a backport of this pull request to the 3.12 branch.

[bedevere-app]

GH-142141 is a backport of this pull request to the 3.11 branch.

[bedevere-app]

GH-142142 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x (tier-1) has failed when building commit 5a4c4a0.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/345/builds/12794) and take a look at the build logs.
4. Check if the failure is related to this commit (5a4c4a0) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/345/builds/12794

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last): File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/support/__init__.py", line 847, in gc_collect gc.collect() ~~~~~~~~~~^^ResourceWarning: unclosed file <_io.FileIO name=13 mode='wb' closefd=True> Traceback (most recent call last): File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/support/__init__.py", line 847, in gc_collect gc.collect() ~~~~~~~~~~^^ResourceWarning: unclosed file <_io.FileIO name=11 mode='wb' closefd=True>

[illia-v]

This is a different kind of error, less critical. You can easily catch OverflowError in the user code, or clip the limit to the reasonable size (it should not be larger than the buffer size) before call. It will not lead to overallocation.

I see, thanks for explaining and fixing the current issue!