gh-43521: Decode gzip responses with gzip.GzipFile

Replace the hand-written _GzipReader with gzip.GzipFile, as requested
in review. Unlike _GzipReader, GzipFile bounds how much a sized read
decompresses. GzipFile does not close the file object it wraps, so
addclosehook closes the response.

gzip is imported inside the handler, so importing urllib.request does
not import it. Junk after the last gzip member now raises BadGzipFile,
as it does with gzip.open(), so the test that expected it to be
ignored is removed.

Author: soreavis

Lib/test/test_urllib2.py

@@ -2123,15 +2123,6 @@ def test_multiple_gzip_members_decoded(self):
         with h.http_response(req, response) as result:
             self.assertEqual(result.read(), b"AAABBB")
 
-    def test_trailing_junk_after_member_ignored(self):
-        # Bytes after the final member that are not another gzip member are
-        # tolerated: the body decodes without error and is not corrupted.
-        h, req = self.opened_request()
-        comp = self.gzip_bytes(self.body) + b"trailing garbage"
-        response = self.make_response(comp)
-        with h.http_response(req, response) as result:
-            self.assertEqual(result.read(), self.body)
-
     def test_user_accept_encoding_passes_through(self):
         # A caller-supplied Accept-Encoding is left to the caller to decode:
         # the gzipped response is returned untouched.
@@ -2164,7 +2155,7 @@ def test_close_propagates(self):
         self.assertTrue(response.closed)
 
     def test_requires_zlib(self):
-        with mock.patch.object(urllib.request, "zlib", None):
+        with mock.patch.dict(sys.modules, {"gzip": None}):
             with self.assertRaises(ImportError):
                 urllib.request.HTTPGzipHandler()
 

Lib/urllib/request.py

@@ -114,11 +114,6 @@
 else:
     _have_ssl = True
 
-try:
-    import zlib
-except ImportError:
-    zlib = None
-
 __all__ = [
     # Classes
     'Request', 'OpenerDirector', 'BaseHandler', 'HTTPDefaultErrorHandler',
@@ -1418,70 +1413,6 @@ def http_response(self, request, response):
     https_response = http_response
 
 
-class _GzipReader(io.BufferedIOBase):
-    """Incrementally decode a gzip Content-Encoding response body."""
-
-    def __init__(self, fp):
-        self._fp = fp
-        self._decompressor = zlib.decompressobj(16 + zlib.MAX_WBITS)
-        self._buffer = b''
-        self._leftover = b''
-        self._got_data = False
-        self._eof = False
-
-    def readable(self):
-        return True
-
-    def _refill(self):
-        while True:
-            raw = self._leftover or self._fp.read(io.DEFAULT_BUFFER_SIZE)
-            self._leftover = b''
-            if not raw:
-                self._eof = True
-                data = self._decompressor.flush()
-                if self._got_data and not self._decompressor.eof:
-                    raise EOFError('compressed response ended before the '
-                                   'end-of-stream marker was reached')
-                return data
-            self._got_data = True
-            if self._decompressor.eof:
-                # The previous member ended; the remaining bytes start the
-                # next concatenated member.  Decode it with a fresh
-                # decompressor, and tolerate trailing bytes that are not
-                # another member, as the reader did before.
-                self._decompressor = zlib.decompressobj(16 + zlib.MAX_WBITS)
-                try:
-                    data = self._decompressor.decompress(raw)
-                except zlib.error:
-                    self._eof = True
-                    return b''
-            else:
-                data = self._decompressor.decompress(raw)
-            self._leftover = self._decompressor.unused_data
-            if data:
-                return data
-
-    def read(self, size=-1):
-        if size is None:
-            size = -1
-        while not self._eof and (size < 0 or len(self._buffer) < size):
-            self._buffer += self._refill()
-        if size < 0:
-            data, self._buffer = self._buffer, b''
-        else:
-            data, self._buffer = self._buffer[:size], self._buffer[size:]
-        return data
-
-    def close(self):
-        try:
-            fp = self._fp
-            if fp is not None:
-                self._fp = None
-                fp.close()
-        finally:
-            super().close()
-
-
 class HTTPGzipHandler(BaseHandler):
     """Request and transparently decode gzip Content-Encoding responses.
 
@@ -1493,8 +1424,8 @@ class HTTPGzipHandler(BaseHandler):
     """
 
     def __init__(self):
-        if zlib is None:
-            raise ImportError('the zlib module is required for gzip decoding')
+        # Raise ImportError now, not on the first response, if zlib is missing.
+        import gzip  # noqa: F401
 
     def http_request(self, request):
         if not request.has_header('Accept-encoding'):
@@ -1505,11 +1436,13 @@ def http_request(self, request):
     def http_response(self, request, response):
         if (getattr(request, '_gzip_injected', False) and
                 response.headers.get('Content-Encoding', '').lower() == 'gzip'):
+            import gzip
             headers = copy.copy(response.headers)
             del headers['Content-Encoding']
             del headers['Content-Length']
-            result = addinfourl(_GzipReader(response), headers,
-                                response.url, response.code)
+            fp = addclosehook(gzip.GzipFile(fileobj=response, mode='rb'),
+                              response.close)
+            result = addinfourl(fp, headers, response.url, response.code)
             result.msg = response.msg
             return result
         return response