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

import io
import time
import tracemalloc


def benchmark_concatenation(n, chunk_size):
    tracemalloc.start()
    start_time = time.time()

    data = b""
    for i in range(n):
        data += bytes([i % 256]) * chunk_size

    end_time = time.time()
    current, peak = tracemalloc.get_traced_memory()
    tracemalloc.stop()

    return peak, end_time - start_time


def benchmark_bytesio(n, chunk_size):
    tracemalloc.start()
    start_time = time.time()

    buffer = io.BytesIO()
    for i in range(n):
        buffer.write(bytes([i % 256]) * chunk_size)

    # getvalue() creates a copy of the buffer content
    result = buffer.getvalue()

    end_time = time.time()
    current, peak = tracemalloc.get_traced_memory()
    tracemalloc.stop()

    return peak, end_time - start_time


def main(n):
    chunk_size = 1024 * 1024  # 1 MB

    print(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__":
    while True:
        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

import io
import time
import tracemalloc


def benchmark_concatenation(n, initial_chunk_size):
    tracemalloc.start()
    start_time = time.time()

    data = b""
    for i in range(n):
        data += bytes([i % 256]) * (initial_chunk_size * (2 ** i))

    end_time = time.time()
    current, peak = tracemalloc.get_traced_memory()
    tracemalloc.stop()

    return peak, end_time - start_time


def benchmark_bytesio(n, initial_chunk_size):
    tracemalloc.start()
    start_time = time.time()

    buffer = io.BytesIO()
    for i in range(n):
        buffer.write(bytes([i % 256]) * (initial_chunk_size * (2 ** i)))

    # getvalue() creates a copy of the buffer content
    result = buffer.getvalue()

    end_time = time.time()
    current, peak = tracemalloc.get_traced_memory()
    tracemalloc.stop()

    return peak, end_time - start_time


def main(n):
    chunk_size = 1024 * 1024  # 1 MB

    print(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__":
    while True:
        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.

[illia-v]

Shouldn't it use min(cursize, amt - cursize) for the self.fp.read call?

[serhiy-storchaka]

Thank you. Yes, this was a copying error, it should be delta (although it worked with cursize too).

[illia-v]

@serhiy-storchaka thank you for the changes!

Could you please help me understand whether this PR is just an alternative to #142022 or both changes are needed?
In the second case, we may need to make a similar change to other methods of HTTPResponse: read1, readinto, readline, and peek

[serhiy-storchaka]

Yes, it is an alternative to #142022.

read1() and peek() don't need a fix, as the underlying buffered reader never reads and returns more than the buffer size. The underlying readline() reads input by chunks (the buffer size for buffered stream and 1 byte for unbuffered stream) and store them in dynamically growing list or byearray.

readinto() does not allocate the buffer for reading. The caller is responsible for allocating a buffer and the vulnerability is in the caller's code.

[illia-v]

Thanks for confirming it's an alternative!

Regarding read1, it's possible to make it raise OverflowError

  File "/usr/lib64/python3.14/http/client.py", line 662, in read1
    return self._read1_chunked(n)
           ~~~~~~~~~~~~~~~~~~~^^^
  File "/usr/lib64/python3.14/http/client.py", line 708, in _read1_chunked
    read = self.fp.read1(n)
OverflowError: Python int too large to convert to C ssize_t

I didn't check if OOM errors can happen too, but I don't see why it's impossible unless there is some special handling of large requested amounts in the underlying reader.

[serhiy-storchaka]

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.

[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!