[agent][clientCertAuth] Fix nil pointer dereference on HTTP response in TryDownload#632
[agent][clientCertAuth] Fix nil pointer dereference on HTTP response in TryDownload#632rustiqly wants to merge 2 commits intosonic-net:masterfrom
Conversation
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#630 Signed-off-by: Rustiqly <rustiqly@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Fixes a panic in the gNMI server’s client certificate CRL download path (TryDownload) by restructuring HTTP error handling to avoid dereferencing a nil *http.Response when http.Get fails.
Changes:
- Return early on
http.Geterror before accessingresp. - Always
defer resp.Body.Close()only after confirming the request succeeded. - Improve logging for non-200 HTTP status responses.
|
|
||
| if resp != nil { | ||
| defer resp.Body.Close() | ||
| if err != nil { |
There was a problem hiding this comment.
In the err != nil early-return path, resp (and resp.Body) may still be non-nil for some net/http failure modes (e.g., redirect-related errors). Returning without closing resp.Body can leak resources. Consider closing the body when resp != nil before returning (guarding resp.Body != nil).
| if err != nil { | |
| if err != nil { | |
| if resp != nil && resp.Body != nil { | |
| resp.Body.Close() | |
| } |
| 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 |
There was a problem hiding this comment.
http.Get uses the default client without a timeout; a slow/non-responsive CRL endpoint can block the auth path indefinitely (potential DoS when CRL checking is enabled). Consider using an http.Client with a reasonable timeout or a context with deadline for the request.
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 <rustiqly@users.noreply.github.com>
|
/azp run |
|
Addressed Copilot review feedback:
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
| glog.Infof("Download CRL: %s failed: %v", url, err) | ||
| return false | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
- There is no nil check here for resp or resp.Body
- If we move this line above line 163, can line163 body be a clean return?
|
@rustiqly please consider adding unit tests. |
|
@yxieca acknowledged, I will work on adding unit tests for this in the next iteration. |
What I did
Fix panic in
TryDownload()whenhttp.Get()returns(nil, err).How I did it
Restructured error handling to check
err != nilfirst and return early before accessingresp. Thendefer resp.Body.Close()and checkresp.StatusCodeseparately.Before:
resp.StatusCodeaccessed in combined check witherr, panics whenrespis nil.After: Error checked first,
responly accessed after confirming non-nil.How to verify it
Call
TryDownload()with an unreachable URL — should return false instead of panic.Which release branch to backport
master
Description for the changelog
Fix nil pointer panic in gNMI CRL download when HTTP request fails.
Fixes: #630