Skip to content

fix: add nil guards to prevent panic in tags package - #416

Closed
Madhan Raj Mookkandy (madhanrm) wants to merge 1 commit into
mainfrom
madhanm/fix-tags-nil-pointer
Closed

fix: add nil guards to prevent panic in tags package#416
Madhan Raj Mookkandy (madhanrm) wants to merge 1 commit into
mainfrom
madhanm/fix-tags-nil-pointer

Conversation

@madhanrm

Copy link
Copy Markdown
Collaborator

Summary

AddTag, AddTagValue, and DeleteTag all dereference the tags pointer without checking for nil. When callers pass nil *common.Tags\ (e.g., \wssdcloudagent/virtualmachineimage/client.go:953\ during VM Create), the functions panic with nil pointer dereference.

Telemetry Evidence

Production telemetry from \HostOsPanicLogs\ (30-day window) shows 4 panics at \ ags.go:58\ (AddTagValue) across 2 devices.

Stack trace:
\
tags.AddTagValue (tags.go:58)
← virtualmachineimage.client.go:953
← virtualmachine.client.go:2029
← VirtualMachineAgent_Invoke_Handler (VM Create)
\\

Changes

  • Add nil guard at the top of \AddTag, \AddTagValue, and \DeleteTag\
  • \GetTagValue\ already handles nil safely via \GetTags()\ proto accessor
  • Added 4 unit tests for nil safety (all functions)

Testing

  • All existing tests pass (\go test ./pkg/tags/ -v)
  • 4 new tests verify nil safety for AddTag, AddTagValue, DeleteTag, GetTagValue

Engineering Checklist Status

Step Status Notes
1. Plan Telemetry-driven investigation
2. Corner Cases Nil tags is the corner case being fixed
3. Error Handling Graceful no-op on nil
4. Error Codes N/A No error codes involved
5. Telemetry Verified via HostOsPanicLogs
6. Test Cases 4 new tests added
7. Pipeline Validation 🔄 Awaiting CICD
8. Green PR 🔄 In progress
9. Feature Flag N/A Bug fix
10. Data-Driven Enablement N/A Bug fix

AddTag, AddTagValue, and DeleteTag all dereference the tags pointer
without checking for nil. When callers pass nil *common.Tags (e.g.,
wssdcloudagent/virtualmachineimage/client.go:953 during VM Create),
the functions panic with nil pointer dereference.

Production telemetry (HostOsPanicLogs, 30-day window) shows 4 panics
at tags.go:58 (AddTagValue) across 2 devices.

Fix: Add nil guard at the top of AddTag, AddTagValue, and DeleteTag.
GetTagValue already handles nil safely via GetTags() proto accessor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@madhanrm

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@madhanrm

Copy link
Copy Markdown
Collaborator Author

Auto-closing this PR as part of the monthly stale-PR cleanup - it has been open for over 90 days or has had no activity in the last 90 days. This is reversible: reactivate the PR if it is still needed. - automated cleanup on behalf of madhanm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant