From 3f8003fd3503ff0344d14359f08d0e3d2c3725e4 Mon Sep 17 00:00:00 2001 From: Rustiqly Date: Wed, 25 Mar 2026 15:08:53 -0700 Subject: [PATCH 1/3] [agent][clientCertAuth] Fix nil pointer dereference on HTTP response http.Get() can return (nil, err). The previous code checked resp != nil only for the defer but then accessed resp.StatusCode in a combined err check, causing a panic when resp is nil. Restructure: check err first and return early, then defer resp.Body.Close() and check StatusCode separately. Fixes: sonic-net/sonic-gnmi#630 Signed-off-by: Rustiqly --- gnmi_server/clientCertAuth.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/gnmi_server/clientCertAuth.go b/gnmi_server/clientCertAuth.go index 381a915e1..5e00f8391 100644 --- a/gnmi_server/clientCertAuth.go +++ b/gnmi_server/clientCertAuth.go @@ -160,12 +160,14 @@ func TryDownload(url string) bool { glog.Infof("Download CRL start: %s", url) resp, err := http.Get(url) - if resp != nil { - defer resp.Body.Close() + if err != nil { + glog.Infof("Download CRL: %s failed: %v", url, err) + return false } + defer resp.Body.Close() - if err != nil || resp.StatusCode != http.StatusOK { - glog.Infof("Download CRL: %s failed: %v", url, err) + if resp.StatusCode != http.StatusOK { + glog.Infof("Download CRL: %s failed: HTTP %d", url, resp.StatusCode) return false } From 8d385abf149156221f6f760ea04e19b9c353dab1 Mon Sep 17 00:00:00 2001 From: Rustiqly Date: Sat, 4 Apr 2026 10:44:44 -0700 Subject: [PATCH 2/3] Close resp.Body on error path to prevent resource leak http.Get may return a non-nil resp with a non-nil Body even when err is non-nil (e.g., redirect-related errors). Close the body before returning to prevent resource leaks. Signed-off-by: Rustiqly --- gnmi_server/clientCertAuth.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gnmi_server/clientCertAuth.go b/gnmi_server/clientCertAuth.go index 5e00f8391..21dee089b 100644 --- a/gnmi_server/clientCertAuth.go +++ b/gnmi_server/clientCertAuth.go @@ -161,6 +161,9 @@ func TryDownload(url string) bool { resp, err := http.Get(url) if err != nil { + if resp != nil && resp.Body != nil { + resp.Body.Close() + } glog.Infof("Download CRL: %s failed: %v", url, err) return false } From 1c19b11b614acb93ecccce9f139cda549014ef52 Mon Sep 17 00:00:00 2001 From: Rustiqly Date: Mon, 11 May 2026 02:15:02 -0700 Subject: [PATCH 3/3] Address CRL download review feedback Signed-off-by: Rustiqly --- gnmi_server/clientCertAuth.go | 8 ++++---- gnmi_server/crl_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/gnmi_server/clientCertAuth.go b/gnmi_server/clientCertAuth.go index 21dee089b..51a240eb0 100644 --- a/gnmi_server/clientCertAuth.go +++ b/gnmi_server/clientCertAuth.go @@ -160,14 +160,14 @@ func TryDownload(url string) bool { glog.Infof("Download CRL start: %s", url) resp, err := http.Get(url) + if resp != nil && resp.Body != nil { + defer resp.Body.Close() + } + if err != nil { - if resp != nil && resp.Body != nil { - resp.Body.Close() - } glog.Infof("Download CRL: %s failed: %v", url, err) return false } - defer resp.Body.Close() if resp.StatusCode != http.StatusOK { glog.Infof("Download CRL: %s failed: HTTP %d", url, resp.StatusCode) diff --git a/gnmi_server/crl_test.go b/gnmi_server/crl_test.go index a77dbb12f..046402a26 100644 --- a/gnmi_server/crl_test.go +++ b/gnmi_server/crl_test.go @@ -12,6 +12,8 @@ import ( "github.com/sonic-net/sonic-gnmi/common_utils" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "net/http" + "net/http/httptest" "os" "testing" "time" @@ -248,3 +250,27 @@ func TestTryDownload(t *testing.T) { t.Errorf("Download should failed: %v", downloaded) } } + +func TestTryDownloadRedirectError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/redirect", http.StatusFound) + })) + defer server.Close() + + downloaded := TryDownload(server.URL) + if downloaded { + t.Errorf("Download should fail on redirect loop") + } +} + +func TestTryDownloadNonOKResponse(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not found", http.StatusNotFound) + })) + defer server.Close() + + downloaded := TryDownload(server.URL) + if downloaded { + t.Errorf("Download should fail on non-OK response") + } +}