Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serhiy-storchaka merged 8 commits into from
Dec 1, 2025
+95 −4
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
25166b1
gh-119451: Fix OOM vulnerability in http.client
serhiy-storchaka May 23, 2024
f097fad
Add also test for non-truncated large body.
serhiy-storchaka May 23, 2024
83cc548
Update 2024-05-23-11-47-48.gh-issue-119451.qkJe9-.rst
serhiy-storchaka Nov 18, 2025
749c7cd
Merge branch 'main' into http-client
serhiy-storchaka Nov 18, 2025
0a33a89
Merge branch 'main' into http-client
serhiy-storchaka Dec 1, 2025
4559a5e
Address review comments.
serhiy-storchaka Dec 1, 2025
1ef9b5a
Fix error.
serhiy-storchaka Dec 1, 2025
5aaf8a5
Optimization.
serhiy-storchaka Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions Lib/http/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@
_MAXLINE = 65536
_MAXHEADERS = 100

# Data larger than this will be read in chunks, to prevent extreme
# overallocation.
_MIN_READ_BUF_SIZE = 1 << 20


# Header name/value ABNF (http://tools.ietf.org/html/rfc7230#section-3.2)
#
# VCHAR = %x21-7E
Expand Down Expand Up @@ -642,9 +647,16 @@ def _safe_read(self, amt):
reading. If the bytes are truly not available (due to EOF), then the
IncompleteRead exception can be used to detect the problem.
"""
data = self.fp.read(amt)
if len(data) < amt:
raise IncompleteRead(data, amt-len(data))
cursize = min(amt, _MIN_READ_BUF_SIZE)
data = self.fp.read(cursize)
while len(data) < amt:
if len(data) < cursize:
raise IncompleteRead(data, amt-len(data))
# This is a geometric increase in read size (never more than
# doubling out the current length of data per loop iteration).
delta = min(cursize, amt - cursize)
data += self.fp.read(cursize)
Copy link
Contributor

@illia-v illia-v Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ")))

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark reads data by chunks with the constant size .

This PR reads it by chunks with geometrically increased size.

Copy link
Contributor

@illia-v illia-v Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ")))

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@illia-v illia-v Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cursize += delta
return data

def _safe_readinto(self, b):
Expand Down
66 changes: 66 additions & 0 deletions Lib/test/test_httplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1511,6 +1511,72 @@ def run_server():
thread.join()
self.assertEqual(result, b"proxied data\n")

def test_large_content_length(self):
serv = socket.create_server((HOST, 0))
self.addCleanup(serv.close)

def run_server():
[conn, address] = serv.accept()
with conn:
while conn.recv(1024):
conn.sendall(
b"HTTP/1.1 200 Ok\r\n"
b"Content-Length: %d\r\n"
b"\r\n" % size)
conn.sendall(b'A' * (size//3))
conn.sendall(b'B' * (size - size//3))

thread = threading.Thread(target=run_server)
thread.start()
self.addCleanup(thread.join, 1.0)

conn = client.HTTPConnection(*serv.getsockname())
try:
for w in range(15, 27):
size = 1 << w
conn.request("GET", "/")
with conn.getresponse() as response:
self.assertEqual(len(response.read()), size)
finally:
conn.close()
thread.join(1.0)

def test_large_content_length_truncated(self):
serv = socket.create_server((HOST, 0))
self.addCleanup(serv.close)

def run_server():
while True:
[conn, address] = serv.accept()
with conn:
conn.recv(1024)
if not size:
break
conn.sendall(
b"HTTP/1.1 200 Ok\r\n"
b"Content-Length: %d\r\n"
b"\r\n"
b"Text" % size)

thread = threading.Thread(target=run_server)
thread.start()
self.addCleanup(thread.join, 1.0)

conn = client.HTTPConnection(*serv.getsockname())
try:
for w in range(18, 65):
size = 1 << w
conn.request("GET", "/")
with conn.getresponse() as response:
self.assertRaises(client.IncompleteRead, response.read)
conn.close()
finally:
conn.close()
size = 0
conn.request("GET", "/")
conn.close()
thread.join(1.0)

def test_putrequest_override_domain_validation(self):
"""
It should be possible to override the default validation
Expand Down
5 changes: 5 additions & 0 deletions Misc/NEWS.d/next/Security/2024-05-23-11-47-48.gh-issue-119451.qkJe9-.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Fix a potential memory denial of service in the :mod:`http.client` module.
When connecting to a malicious server, it could cause
an arbitrary amount of memory to be allocated.
This could have led to symptoms including a :exc:`MemoryError`, swapping, out
of memory (OOM) killed processes or containers, or even system crashes.
Loading