This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 297fb2c1 fix(catalog/rest): do not leak response bodies (#655)
297fb2c1 is described below
commit 297fb2c1e1527c3a106b774e4e6269f40fda3f5d
Author: ferhat elmas <[email protected]>
AuthorDate: Tue Dec 23 16:55:11 2025 +0100
fix(catalog/rest): do not leak response bodies (#655)
In error case, body isn't closed so
underlying TCP connection might not be used.
Defer closing and draining unconditionally
for safer and more idiomatic handling.
Signed-off-by: ferhat elmas <[email protected]>
---
catalog/rest/rest.go | 10 ++++-
catalog/rest/rest_internal_test.go | 79 ++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/catalog/rest/rest.go b/catalog/rest/rest.go
index 1633ffb7..6c1f92b9 100644
--- a/catalog/rest/rest.go
+++ b/catalog/rest/rest.go
@@ -266,6 +266,10 @@ func do[T any](ctx context.Context, method string, baseURI
*url.URL, path []stri
if rsp, err = cl.Do(req); err != nil {
return ret, err
}
+ defer func() {
+ _, _ = io.Copy(io.Discard, rsp.Body)
+ _ = rsp.Body.Close()
+ }()
if allowNoContent && rsp.StatusCode == http.StatusNoContent {
return ret, err
@@ -279,7 +283,6 @@ func do[T any](ctx context.Context, method string, baseURI
*url.URL, path []stri
return ret, err
}
- defer rsp.Body.Close()
if err = json.NewDecoder(rsp.Body).Decode(&ret); err != nil {
return ret, fmt.Errorf("%w: error decoding json payload: `%s`",
ErrRESTError, err.Error())
}
@@ -327,6 +330,10 @@ func doPostAllowNoContent[Payload, Result any](ctx
context.Context, baseURI *url
if err != nil {
return ret, err
}
+ defer func() {
+ _, _ = io.Copy(io.Discard, rsp.Body)
+ _ = rsp.Body.Close()
+ }()
if allowNoContent && rsp.StatusCode == http.StatusNoContent {
return ret, err
@@ -340,7 +347,6 @@ func doPostAllowNoContent[Payload, Result any](ctx
context.Context, baseURI *url
return ret, err
}
- defer rsp.Body.Close()
if err = json.NewDecoder(rsp.Body).Decode(&ret); err != nil {
return ret, fmt.Errorf("%w: error decoding json payload: `%s`",
ErrRESTError, err.Error())
}
diff --git a/catalog/rest/rest_internal_test.go
b/catalog/rest/rest_internal_test.go
index 62135aca..6141ce53 100644
--- a/catalog/rest/rest_internal_test.go
+++ b/catalog/rest/rest_internal_test.go
@@ -465,3 +465,82 @@ func TestSigv4ConcurrentSigners(t *testing.T) {
require.NoError(t, grp.Wait())
t.Logf("issued %d requests", count.Load())
}
+
+// trackingReadCloser wraps an io.ReadCloser to track if Close() was called
+type trackingReadCloser struct {
+ io.ReadCloser
+ closed bool
+}
+
+func (t *trackingReadCloser) Close() error {
+ t.closed = true
+
+ return t.ReadCloser.Close()
+}
+
+// trackingTransport wraps http.RoundTripper to track response bodies
+type trackingTransport struct {
+ transport http.RoundTripper
+ body *trackingReadCloser
+}
+
+func (t *trackingTransport) RoundTrip(req *http.Request) (*http.Response,
error) {
+ resp, err := t.transport.RoundTrip(req)
+ if err != nil {
+ return nil, err
+ }
+
+ t.body = &trackingReadCloser{ReadCloser: resp.Body}
+ resp.Body = t.body
+
+ return resp, nil
+}
+
+// TestResponseBodyLeak checks if response body is closed properly.
+func TestResponseBodyLeak(t *testing.T) {
+ t.Parallel()
+
+ mux := http.NewServeMux()
+ srv := httptest.NewServer(mux)
+ defer srv.Close()
+
+ mux.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": errorResponse{
+ Message: "not found",
+ Type: "NoSuchTableException",
+ Code: 404,
+ },
+ })
+ })
+
+ t.Run("do", func(t *testing.T) {
+ tracker := &trackingTransport{transport: http.DefaultTransport}
+ client := &http.Client{Transport: tracker}
+
+ baseURI, err := url.Parse(srv.URL)
+ require.NoError(t, err)
+
+ _, err = do[struct{}](context.Background(), http.MethodGet,
baseURI, []string{"test"}, client, nil, false)
+ require.Error(t, err)
+
+ assert.True(t, tracker.body.closed,
+ "response body should be closed on non-200 status")
+ })
+
+ t.Run("doPostAllowNoContent", func(t *testing.T) {
+ tracker := &trackingTransport{transport: http.DefaultTransport}
+ client := &http.Client{Transport: tracker}
+
+ baseURI, err := url.Parse(srv.URL)
+ require.NoError(t, err)
+
+ _, err = doPostAllowNoContent[map[string]string, struct{}](
+ context.Background(), baseURI, []string{"test"},
map[string]string{"key": "value"}, client, nil, false)
+ require.Error(t, err)
+
+ assert.True(t, tracker.body.closed,
+ "response body should be closed on non-200 status")
+ })
+}
