From 9878730e20d68eb2a04ed82c5fffff9ab8f94d02 Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Tue, 27 Feb 2024 20:48:50 +0200 Subject: [PATCH 1/8] gh-115997: Make `HTTPResponse.read1` and `readline` raise `IncompleteRead` --- Lib/http/client.py | 4 ++++ Lib/test/test_httplib.py | 24 +++++++++++++++++++ ...-02-27-20-56-44.gh-issue-115997.LqLP4V.rst | 4 ++++ 3 files changed, 32 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst diff --git a/Lib/http/client.py b/Lib/http/client.py index a353716a8506e65..4a5f97e5ff8bcc1 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -663,6 +663,8 @@ def read1(self, n=-1): result = self.fp.read1(n) if not result and n: self._close_conn() + if self.length: + raise IncompleteRead(result, self.length) elif self.length is not None: self.length -= len(result) if not self.length: @@ -689,6 +691,8 @@ def readline(self, limit=-1): result = self.fp.readline(limit) if not result and limit: self._close_conn() + if self.length: + raise IncompleteRead(result, self.length) elif self.length is not None: self.length -= len(result) if not self.length: diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index 6e63a8872d9c6e1..eb776833df4d2a9 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1608,6 +1608,30 @@ class ExtendedReadTestContentLengthKnown(ExtendedReadTest): _header, _body = ExtendedReadTest.lines.split('\r\n\r\n', 1) lines = _header + f'\r\nContent-Length: {len(_body)}\r\n\r\n' + _body + def _test_incomplete_read(self, read_meth): + resp = self.resp + # Reduce the size of content the response object will read to + # cause the incomplete read. + resp.fp.read(1) + with self.assertRaises(client.IncompleteRead) as cm: + while True: + data = resp.read1() + if not data: + break + exception = cm.exception + self.assertEqual(exception.partial, b"") + self.assertEqual(exception.expected, 1) + self.assertTrue(resp.isclosed()) + + def test_read_incomplete_read(self): + self._test_incomplete_read(self.resp.read) + + def test_read1_incomplete_read(self): + self._test_incomplete_read(self.resp.read1) + + def test_readline_incomplete_read(self): + self._test_incomplete_read(self.resp.readline) + class ExtendedReadTestChunked(ExtendedReadTest): """ diff --git a/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst b/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst new file mode 100644 index 000000000000000..f33feb0390d1ccc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst @@ -0,0 +1,4 @@ +Make ``http.client.HTTPResponse.read1`` and +``http.client.HTTPResponse.readline`` raise :exc:`IncompleteRead` instead of +returning zero bytes if a connection is closed before an expected number of +bytes has been read. Patch by Illia Volochii. From 9f8f278604a5a0ec068d604d48c93a97e993bb4c Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Tue, 27 Feb 2024 21:05:57 +0200 Subject: [PATCH 2/8] Fix the new tests --- Lib/test/test_httplib.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index eb776833df4d2a9..c04159f4bfb6679 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1615,11 +1615,21 @@ def _test_incomplete_read(self, read_meth): resp.fp.read(1) with self.assertRaises(client.IncompleteRead) as cm: while True: - data = resp.read1() + data = read_meth() if not data: break exception = cm.exception - self.assertEqual(exception.partial, b"") + # Unlike `read1` and `readline`, `read` tries to read the whole + # content during one call, so it's partial is not empty in this + # case. + # `read1` and `readline` raise `IncompleteRead` only when they + # read 0 bytes before all expected content has been read, so the + # partial is empty. + if read_meth == self.resp.read: + expected_partial = self._body[1:].encode() + else: + expected_partial = b"" + self.assertEqual(exception.partial, expected_partial) self.assertEqual(exception.expected, 1) self.assertTrue(resp.isclosed()) From 33f7d775821a5de7a4c45636d38ba7b1463baf3f Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Mon, 10 Feb 2025 20:54:03 +0200 Subject: [PATCH 3/8] Do not set `expected` to new `IncompleteRead` --- Lib/http/client.py | 4 ++-- Lib/test/test_httplib.py | 13 ++++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/Lib/http/client.py b/Lib/http/client.py index 81ca7fca53b5de1..819654d62fd12d5 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -666,7 +666,7 @@ def read1(self, n=-1): if not result and n: self._close_conn() if self.length: - raise IncompleteRead(result, self.length) + raise IncompleteRead(result) elif self.length is not None: self.length -= len(result) if not self.length: @@ -694,7 +694,7 @@ def readline(self, limit=-1): if not result and limit: self._close_conn() if self.length: - raise IncompleteRead(result, self.length) + raise IncompleteRead(result) elif self.length is not None: self.length -= len(result) if not self.length: diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index 13a1acd678ac654..4d847bb368f3822 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1641,7 +1641,7 @@ class ExtendedReadTestContentLengthKnown(ExtendedReadTest): _header, _body = ExtendedReadTest.lines.split('\r\n\r\n', 1) lines = _header + f'\r\nContent-Length: {len(_body)}\r\n\r\n' + _body - def _test_incomplete_read(self, read_meth): + def _test_incomplete_read(self, read_meth, expected_none): resp = self.resp # Reduce the size of content the response object will read to # cause the incomplete read. @@ -1663,17 +1663,20 @@ def _test_incomplete_read(self, read_meth): else: expected_partial = b"" self.assertEqual(exception.partial, expected_partial) - self.assertEqual(exception.expected, 1) + if expected_none: + self.assertIsNone(exception.expected) + else: + self.assertEqual(exception.expected, 1) self.assertTrue(resp.isclosed()) def test_read_incomplete_read(self): - self._test_incomplete_read(self.resp.read) + self._test_incomplete_read(self.resp.read, expected_none=False) def test_read1_incomplete_read(self): - self._test_incomplete_read(self.resp.read1) + self._test_incomplete_read(self.resp.read1, expected_none=True) def test_readline_incomplete_read(self): - self._test_incomplete_read(self.resp.readline) + self._test_incomplete_read(self.resp.readline, expected_none=True) class ExtendedReadTestChunked(ExtendedReadTest): From 26b43e16a0580bbeb6902cb15e329138f53756b8 Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Mon, 10 Feb 2025 23:03:57 +0200 Subject: [PATCH 4/8] Handle a case with a not reached newline --- Lib/http/client.py | 3 ++ Lib/test/test_httplib.py | 33 ++++++++++++++++++- ...-02-27-20-56-44.gh-issue-115997.LqLP4V.rst | 7 ++-- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/Lib/http/client.py b/Lib/http/client.py index 819654d62fd12d5..5abaabdd3de096a 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -699,6 +699,9 @@ def readline(self, limit=-1): self.length -= len(result) if not self.length: self._close_conn() + elif len(result) != limit and not result.endswith(b"\n"): + self._close_conn() + raise IncompleteRead(result) return result def _read1_chunked(self, n): diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index 4d847bb368f3822..2cf0cd9b8c8e2bf 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1675,9 +1675,40 @@ def test_read_incomplete_read(self): def test_read1_incomplete_read(self): self._test_incomplete_read(self.resp.read1, expected_none=True) - def test_readline_incomplete_read(self): + def test_readline_incomplete_read_with_complete_line(self): + """ + Test that IncompleteRead is raised when readline finishes + reading a response but the needed content length is not reached. + """ + resp = self.resp + content = resp.fp.read() + # For this test case, we must ensure that the last byte read + # will be a newline. There is a different handling of readline + # not reaching a newline. + content = content[:-1] + b"\n" + resp.fp = io.BytesIO(content) self._test_incomplete_read(self.resp.readline, expected_none=True) + def test_readline_incomplete_read_with_incomplete_line(self): + """ + Test that IncompleteRead is raised when readline is expected + to read a line fully but a newline is not reached. + """ + resp = self.resp + content = resp.fp.read() + # Truncate the content to the last newline. + content = content[:content.rindex(b"\n") - 1] + resp.fp = io.BytesIO(content) + with self.assertRaises(client.IncompleteRead) as cm: + while True: + data = resp.readline() + if not data: + break + exception = cm.exception + self.assertEqual(exception.partial, content.split(b"\n")[-1]) + self.assertIsNone(exception.expected) + self.assertTrue(resp.isclosed()) + class ExtendedReadTestChunked(ExtendedReadTest): """ diff --git a/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst b/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst index f33feb0390d1ccc..9fd6e790ce4af98 100644 --- a/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst +++ b/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst @@ -1,4 +1,5 @@ Make ``http.client.HTTPResponse.read1`` and -``http.client.HTTPResponse.readline`` raise :exc:`IncompleteRead` instead of -returning zero bytes if a connection is closed before an expected number of -bytes has been read. Patch by Illia Volochii. +``http.client.HTTPResponse.readline`` raise :exc:`http.client.IncompleteRead` +instead of returning zero bytes if a connection is closed before an expected +number of bytes has been read. Also, ``http.client.HTTPResponse.readline`` +raises when an expected newline has not been reached. Patch by Illia Volochii. From 0b6c92983e6bd74611c9c8959d993874bc50d9f1 Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Tue, 1 Apr 2025 20:47:44 +0300 Subject: [PATCH 5/8] Refactor `test_readline_incomplete_read_with_incomplete_line` --- Lib/test/test_httplib.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index 2cf0cd9b8c8e2bf..3ec1b235e7dc696 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1696,8 +1696,8 @@ def test_readline_incomplete_read_with_incomplete_line(self): """ resp = self.resp content = resp.fp.read() - # Truncate the content to the last newline. - content = content[:content.rindex(b"\n") - 1] + # Remove last newline and following bytes. + content = content.rsplit(b'\n', 1)[0] resp.fp = io.BytesIO(content) with self.assertRaises(client.IncompleteRead) as cm: while True: @@ -1705,7 +1705,7 @@ def test_readline_incomplete_read_with_incomplete_line(self): if not data: break exception = cm.exception - self.assertEqual(exception.partial, content.split(b"\n")[-1]) + self.assertEqual(exception.partial, content.rsplit(b'\n', 1)[1]) self.assertIsNone(exception.expected) self.assertTrue(resp.isclosed()) From 31fc0ccd425e4ade03482f1aa94edec8aa09e0e8 Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Sat, 18 Jul 2026 14:25:45 +0000 Subject: [PATCH 6/8] Apply the same change for `read(N)` and `readinto` Co-authored-by: Seth Michael Larson --- Lib/http/client.py | 8 +++---- Lib/test/test_httplib.py | 21 +++++++++++++------ ...-02-27-20-56-44.gh-issue-115997.LqLP4V.rst | 7 ++++--- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/Lib/http/client.py b/Lib/http/client.py index b11a66642433f61..6a6461ccec11600 100644 --- a/Lib/http/client.py +++ b/Lib/http/client.py @@ -502,9 +502,9 @@ def read(self, amt=None): 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() + if self.length: + raise IncompleteRead(s) elif self.length is not None: self.length -= len(s) if not self.length: @@ -549,9 +549,9 @@ def readinto(self, b): # (for example, reading in 1k chunks) n = self.fp.readinto(b) if not n and b: - # Ideally, we would raise IncompleteRead if the content-length - # wasn't satisfied, but it might break compatibility. self._close_conn() + if self.length: + raise IncompleteRead(b"") elif self.length is not None: self.length -= n if not self.length: diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index 782fe58a9ea1389..a12cecf3d69052d 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -8,6 +8,7 @@ import re import socket import threading +from functools import partial import unittest from unittest import mock @@ -1012,7 +1013,11 @@ def test_partial_reads_incomplete_body(self): self.assertEqual(resp.read(2), b'Te') self.assertFalse(resp.isclosed()) self.assertEqual(resp.read(2), b'xt') - self.assertEqual(resp.read(1), b'') + with self.assertRaises(client.IncompleteRead) as cm: + resp.read(1) + exception = cm.exception + self.assertEqual(exception.partial, b"") + self.assertIsNone(exception.expected) self.assertTrue(resp.isclosed()) def test_partial_readintos_incomplete_body(self): @@ -1030,12 +1035,12 @@ def test_partial_readintos_incomplete_body(self): n = resp.readinto(b) self.assertEqual(n, 2) self.assertEqual(bytes(b), b'xt') - n = resp.readinto(b) - self.assertEqual(n, 0) + with self.assertRaises(client.IncompleteRead) as cm: + resp.readinto(b) + exception = cm.exception + self.assertEqual(exception.partial, b"") + self.assertIsNone(exception.expected) self.assertTrue(resp.isclosed()) - self.assertFalse(resp.closed) - resp.close() - self.assertTrue(resp.closed) def test_host_port(self): # Check invalid host_port @@ -1915,6 +1920,10 @@ def _test_incomplete_read(self, read_meth, expected_none): def test_read_incomplete_read(self): self._test_incomplete_read(self.resp.read, expected_none=False) + def test_read_incomplete_read_n(self): + read_meth = partial(self.resp.read, len(self._body)) + self._test_incomplete_read(read_meth, expected_none=True) + def test_read1_incomplete_read(self): self._test_incomplete_read(self.resp.read1, expected_none=True) diff --git a/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst b/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst index 9fd6e790ce4af98..628ef7cdbce4bb7 100644 --- a/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst +++ b/Misc/NEWS.d/next/Library/2024-02-27-20-56-44.gh-issue-115997.LqLP4V.rst @@ -1,5 +1,6 @@ -Make ``http.client.HTTPResponse.read1`` and -``http.client.HTTPResponse.readline`` raise :exc:`http.client.IncompleteRead` +Make ``read(N)``, ``read1``, ``readinto`` and ``readline`` of +``http.client.HTTPResponse`` raise :exc:`http.client.IncompleteRead` instead of returning zero bytes if a connection is closed before an expected number of bytes has been read. Also, ``http.client.HTTPResponse.readline`` -raises when an expected newline has not been reached. Patch by Illia Volochii. +raises when an expected newline has not been reached. +Patch by Illia Volochii and Seth Larson. From e7fd00e64ea8254459a017de35fa6fc7a277e587 Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Sat, 18 Jul 2026 16:01:01 +0000 Subject: [PATCH 7/8] Restore `ContentTooShortError` for urllib --- Lib/urllib/request.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Lib/urllib/request.py b/Lib/urllib/request.py index 660301fef612588..5c43eae78397f4b 100644 --- a/Lib/urllib/request.py +++ b/Lib/urllib/request.py @@ -237,7 +237,15 @@ def urlretrieve(url, filename=None, reporthook=None, data=None): if reporthook: reporthook(blocknum, bs, size) - while block := fp.read(bs): + while True: + try: + block = fp.read(bs) + except http.client.IncompleteRead: + # This case has been handled by ContentTooShortError + # below historically. + break + if not block: + break read += len(block) tfp.write(block) blocknum += 1 From eaba1d5940a22b8a3f39613d04f2c190d5aac30c Mon Sep 17 00:00:00 2001 From: Illia Volochii Date: Sat, 18 Jul 2026 22:30:27 +0000 Subject: [PATCH 8/8] Restore test asserts dropped accidentally --- Lib/test/test_httplib.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/test/test_httplib.py b/Lib/test/test_httplib.py index a12cecf3d69052d..db2f865a3232c79 100644 --- a/Lib/test/test_httplib.py +++ b/Lib/test/test_httplib.py @@ -1041,6 +1041,9 @@ def test_partial_readintos_incomplete_body(self): self.assertEqual(exception.partial, b"") self.assertIsNone(exception.expected) self.assertTrue(resp.isclosed()) + self.assertFalse(resp.closed) + resp.close() + self.assertTrue(resp.closed) def test_host_port(self): # Check invalid host_port