Skip to content

Commit c627d78

Browse files
authored
[scanner] fix: add nil-safety guards to prevent potential panics (#19238)
Fixes #19194\n\nAdds nil checks to prevent potential nil pointer panics identified by nilaway static analysis:\n- pkg/stellar/solver/solver.go: nil check for deployment object\n- pkg/mcp/client.go: nil check for response\n- pkg/agent/workers/workers_device_tracker.go: nil checks for alert pointers\n- pkg/k8s/client_clients.go: nil checks for rest.CopyConfig return\n- Test files: added require.NotNil guards
1 parent fa53325 commit c627d78

8 files changed

Lines changed: 27 additions & 6 deletions

File tree

pkg/agent/broadcast_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ func TestBroadcastToClients_RemovesDeadClients(t *testing.T) {
102102
defer cleanup()
103103

104104
// Close client 0's underlying TCP connection to simulate a dead peer.
105-
if clients[0] != nil {
105+
if len(clients) > 0 && clients[0] != nil {
106106
if uc := clients[0].UnderlyingConn(); uc != nil {
107107
uc.Close()
108108
}

pkg/agent/federation/providers/test_server_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,9 @@ func writeStatusResponse(w http.ResponseWriter, code int, path string) {
158158
}
159159

160160
func mergeMaps(dst, patch map[string]interface{}) {
161+
if dst == nil || patch == nil {
162+
return
163+
}
161164
for key, value := range patch {
162165
patchMap, patchIsMap := value.(map[string]interface{})
163166
dstMap, dstIsMap := dst[key].(map[string]interface{})

pkg/agent/workers/workers_device_tracker.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,7 @@ func (t *DeviceTracker) checkForDrop(key, nodeName, cluster, deviceType string,
372372
}
373373

374374
now := time.Now()
375-
if existing, ok := t.alerts[alertKey]; ok {
375+
if existing, ok := t.alerts[alertKey]; ok && existing != nil {
376376
existing.CurrentCount = currentCount
377377
existing.DroppedCount = dropped
378378
existing.LastSeen = now
@@ -417,7 +417,7 @@ func (t *DeviceTracker) checkForBoolDrop(key, nodeName, cluster, deviceType stri
417417
severity = "critical" // Driver issues are critical
418418
}
419419

420-
if existing, ok := t.alerts[alertKey]; ok {
420+
if existing, ok := t.alerts[alertKey]; ok && existing != nil {
421421
existing.LastSeen = now
422422
existing.Severity = severity
423423
return existing

pkg/api/handlers/github/pipelines_client_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ func TestGHGet_BuildsCorrectURL(t *testing.T) {
2929

3030
resp, err := handler.ghGet(context.Background(), "/repos/test/repo/actions/runs")
3131
require.NoError(t, err)
32+
require.NotNil(t, resp)
3233
assert.Equal(t, http.StatusOK, resp.StatusCode)
3334
}
3435

@@ -49,6 +50,7 @@ func TestGHGet_HandlesFullURL(t *testing.T) {
4950

5051
resp, err := handler.ghGet(context.Background(), "https://api.github.com/repos/test/repo")
5152
require.NoError(t, err)
53+
require.NotNil(t, resp)
5254
assert.Equal(t, http.StatusOK, resp.StatusCode)
5355
}
5456

@@ -68,9 +70,7 @@ func TestGHGetWithRetry_SuccessFirstAttempt(t *testing.T) {
6870

6971
resp, err := handler.ghGetWithRetry(context.Background(), "/test")
7072
require.NoError(t, err)
71-
if resp == nil {
72-
t.Fatal("expected non-nil response")
73-
}
73+
require.NotNil(t, resp)
7474
assert.Equal(t, http.StatusOK, resp.StatusCode)
7575
}
7676

@@ -99,6 +99,7 @@ func TestGHGetWithRetry_RateLimitRetry(t *testing.T) {
9999

100100
resp, err := handler.ghGetWithRetry(context.Background(), "/test")
101101
require.NoError(t, err)
102+
require.NotNil(t, resp)
102103
assert.Equal(t, http.StatusOK, resp.StatusCode)
103104
assert.Equal(t, 2, attemptCount, "should retry after rate limit")
104105
}
@@ -121,6 +122,7 @@ func TestGHGetWithRetry_MaxAttemptsExceeded(t *testing.T) {
121122

122123
resp, err := handler.ghGetWithRetry(context.Background(), "/test")
123124
require.NoError(t, err) // Function returns the response, not an error
125+
require.NotNil(t, resp)
124126
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
125127
// Should have attempted the maximum number of times
126128
assert.True(t, attemptCount >= 1, "should make at least one attempt")
@@ -156,6 +158,7 @@ func TestGHGetWithRetry_RespectsRetryAfterHeader(t *testing.T) {
156158
elapsed := time.Since(start)
157159

158160
require.NoError(t, err)
161+
require.NotNil(t, resp)
159162
assert.Equal(t, http.StatusOK, resp.StatusCode)
160163
// Should have waited at least 1 second due to Retry-After header
161164
assert.True(t, elapsed >= time.Second, "should respect Retry-After header")

pkg/k8s/client_clients.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ func (m *MultiClusterClient) GetClient(contextName string) (kubernetes.Interface
3737
isInCluster := inClusterConfig != nil && (contextName == "in-cluster" || contextName == inClusterName)
3838
if isInCluster {
3939
config = rest.CopyConfig(inClusterConfig)
40+
// Guard against nil return from rest.CopyConfig (#19194).
41+
if config == nil {
42+
return nil, fmt.Errorf("failed to copy in-cluster config for context %s", contextName)
43+
}
4044
} else {
4145
config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
4246
&clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath},
@@ -127,6 +131,10 @@ func (m *MultiClusterClient) GetDynamicClient(contextName string) (dynamic.Inter
127131
isInCluster := inClusterConfig != nil && (contextName == "in-cluster" || contextName == inClusterName)
128132
if isInCluster {
129133
config = rest.CopyConfig(inClusterConfig)
134+
// Guard against nil return from rest.CopyConfig (#19194).
135+
if config == nil {
136+
return nil, fmt.Errorf("failed to copy in-cluster config for context %s", contextName)
137+
}
130138
} else {
131139
config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
132140
&clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath},

pkg/mcp/client.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,9 @@ func (c *Client) call(ctx context.Context, method string, params interface{}) (j
426426
}
427427
return nil, fmt.Errorf("context done")
428428
case resp := <-respCh:
429+
if resp == nil {
430+
return nil, fmt.Errorf("nil response from channel")
431+
}
429432
if resp.Error != nil {
430433
return nil, fmt.Errorf("RPC error %d: %s", resp.Error.Code, resp.Error.Message)
431434
}

pkg/stellar/solver/solver.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,9 @@ func verifyResourceHealth(ctx context.Context, k8sClient *k8s.MultiClusterClient
285285
if err != nil {
286286
return false, fmt.Sprintf("deployment read error: %s", err.Error())
287287
}
288+
if deploy == nil {
289+
return false, "deployment read returned nil"
290+
}
288291
if deploy.Status.ReadyReplicas > 0 && deploy.Status.ReadyReplicas == deploy.Status.Replicas {
289292
return true, fmt.Sprintf("%d/%d replicas ready.", deploy.Status.ReadyReplicas, deploy.Status.Replicas)
290293
}

pkg/store/sqlite_users_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ func TestSQLiteStore_UpdateUserRole(t *testing.T) {
217217

218218
got, err := s.GetUser(ctx, user.ID)
219219
require.NoError(t, err)
220+
require.NotNil(t, got)
220221
assert.Equal(t, models.UserRoleAdmin, got.Role)
221222
}
222223

0 commit comments

Comments
 (0)