From d131cf990b02ce66424d77c19dff121f18145b2a Mon Sep 17 00:00:00 2001 From: Vitaliy Vasylenko Date: Wed, 12 Nov 2025 11:57:17 -0500 Subject: [PATCH 1/3] fix: descriptor leaking in salt.utils.http.query --- salt/utils/http.py | 11 +++++++---- tests/pytests/unit/utils/test_http.py | 3 +++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/salt/utils/http.py b/salt/utils/http.py index 9dfc1d91b2b6..116f3f313424 100644 --- a/salt/utils/http.py +++ b/salt/utils/http.py @@ -615,12 +615,15 @@ def query( req_kwargs = salt.utils.data.decode(req_kwargs, to_str=True) try: - download_client = SyncWrapper( + download_client_kwargs = ( + {"max_body_size": max_body} if supports_max_body_size else {} + ) + with SyncWrapper( AsyncHTTPClient, - kwargs={"max_body_size": max_body} if supports_max_body_size else {}, + kwargs=download_client_kwargs, async_methods=["fetch"], - ) - result = download_client.fetch(url_full, **req_kwargs) + ) as download_client: + result = download_client.fetch(url_full, **req_kwargs) except tornado.httpclient.HTTPError as exc: ret["status"] = exc.code ret["error"] = str(exc) diff --git a/tests/pytests/unit/utils/test_http.py b/tests/pytests/unit/utils/test_http.py index 90e08c520b18..48fe889698b5 100644 --- a/tests/pytests/unit/utils/test_http.py +++ b/tests/pytests/unit/utils/test_http.py @@ -183,6 +183,9 @@ def test_query_tornado_httperror_no_response(): mock_client = MagicMock() mock_client.fetch.side_effect = http_error + # http.query() uses SyncWrapper as a context manager; ensure + # __enter__() returns the mock_client itself. + mock_client.__enter__.return_value = mock_client # http.query() wraps tornado's AsyncHTTPClient in a SyncWrapper before # calling .fetch(); patch the wrapper so .fetch() raises the HTTPError. From 98a763c203c94627bebdeeed4137184c7e2b3ee9 Mon Sep 17 00:00:00 2001 From: Vitaliy Vasylenko Date: Wed, 12 Nov 2025 12:20:20 -0500 Subject: [PATCH 2/3] chore: add tests --- tests/pytests/unit/utils/test_http.py | 91 +++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/tests/pytests/unit/utils/test_http.py b/tests/pytests/unit/utils/test_http.py index 48fe889698b5..51d47cfbb374 100644 --- a/tests/pytests/unit/utils/test_http.py +++ b/tests/pytests/unit/utils/test_http.py @@ -3,6 +3,7 @@ import pytest import requests +import tornado.httpclient from pytestshellutils.utils import ports from werkzeug.wrappers import Response # pylint: disable=3rd-party-module-not-gated @@ -10,6 +11,62 @@ from tests.support.mock import MagicMock, patch +class _FakeFetchResponse: + """Simple object mimicking the bits of tornado's HTTPResponse we rely on.""" + + def __init__(self, code=200, body=b"payload", headers=None): + self.code = code + self.body = body + self.headers = headers or {"Content-Type": "text/plain; charset=utf-8"} + + +@pytest.fixture +def syncwrapper_stub(monkeypatch): + """Patch ``salt.utils.http.SyncWrapper`` with a controllable test double.""" + + class SyncWrapperStub: + fetch_return = _FakeFetchResponse() + fetch_side_effect = None + enter_calls = 0 + close_calls = 0 + fetch_calls = 0 + + def __init__(self, *args, **kwargs): + # Mirror SyncWrapper signature but we only need to track usage. + pass + + def __enter__(self): + SyncWrapperStub.enter_calls += 1 + return self + + def __exit__(self, exc_type, exc, tb): + self.close() + # Propagate exceptions so http.query can handle them + return False + + def close(self): + SyncWrapperStub.close_calls += 1 + + def fetch(self, *args, **kwargs): + SyncWrapperStub.fetch_calls += 1 + if SyncWrapperStub.fetch_side_effect is not None: + raise SyncWrapperStub.fetch_side_effect + return SyncWrapperStub.fetch_return + + @classmethod + def reset_counters(cls, *, clear_fetch=True): + cls.enter_calls = 0 + cls.close_calls = 0 + cls.fetch_calls = 0 + if clear_fetch: + cls.fetch_side_effect = None + cls.fetch_return = _FakeFetchResponse() + + monkeypatch.setattr(http, "SyncWrapper", SyncWrapperStub) + SyncWrapperStub.reset_counters() + return SyncWrapperStub + + def test_requests_session_verify_ssl_false(ssl_webserver, integration_files_dir): """ test salt.utils.http.session when using verify_ssl @@ -198,6 +255,40 @@ def test_query_tornado_httperror_no_response(): assert "body" not in ret +def test_query_tornado_closes_syncwrapper_on_success(syncwrapper_stub): + syncwrapper_stub.reset_counters() + syncwrapper_stub.fetch_return = _FakeFetchResponse(body=b"test-body") + + ret = http.query("http://example.com", backend="tornado", status=True) + + assert syncwrapper_stub.enter_calls == 1 + assert syncwrapper_stub.close_calls == 1 + assert syncwrapper_stub.fetch_calls == 1 + assert ret["body"] == "test-body" + assert ret["status"] == 200 + + +def test_query_tornado_closes_syncwrapper_on_http_error(syncwrapper_stub): + syncwrapper_stub.reset_counters() + response = MagicMock(body=b"", headers={"Content-Type": "text/plain"}) + syncwrapper_stub.fetch_side_effect = tornado.httpclient.HTTPError( + 599, "Unit test failure", response=response + ) + + ret = http.query( + "http://example.com", + backend="tornado", + status=True, + raise_error=True, + ) + + assert syncwrapper_stub.enter_calls == 1 + assert syncwrapper_stub.close_calls == 1 + assert syncwrapper_stub.fetch_calls == 1 + assert ret["status"] == 599 + assert "Unit test failure" in ret["error"] + + def test_parse_cookie_header(): header = "; ".join( [ From f5172a50dafeb064c2a29a867cf84d7f2a441b33 Mon Sep 17 00:00:00 2001 From: Vitaliy Vasylenko Date: Fri, 14 Nov 2025 09:54:31 -0500 Subject: [PATCH 3/3] chore: add changelog --- changelog/68456.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/68456.fixed.md diff --git a/changelog/68456.fixed.md b/changelog/68456.fixed.md new file mode 100644 index 000000000000..2aef5acaf4f7 --- /dev/null +++ b/changelog/68456.fixed.md @@ -0,0 +1 @@ +Fixed descriptor leaking in salt.utils.http.query