potential nil ptr dereference in (*Dispatcher).processUpdates (SAST warning)
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 3.7k
- Forks
- 676
- Avg merge
- 4d 9h
- Merged PRs (30d)
- 6
Description
Hello!
I used SAST tool Svace to analyze swarmkit (master branch, latest commit at the moment is a45be3cac15cc8321f6214262d173f987e3b55f9) and ecnountered warning about potential nil ptr dereference in manager/dispatcher/dispatcher.go/(*Dispatcher).processUpdates. In particular, in code:
for volumeID, nodes := range unpublishedVolumes {
err := batch.Update(func(tx store.Tx) error {
logger := logr.WithField("volume.id", volumeID)
volume := store.GetVolume(tx, volumeID)
if volume == nil {
logger.Error("volume unavailable")
}
// buckle your seatbelts, we're going quadratic.
nodesLoop:
for _, nodeID := range nodes {
for _, status := range volume.PublishStatus {
if status.NodeID == nodeID {
status.State = api.VolumePublishStatus_PENDING_UNPUBLISH
continue nodesLoop
}
}
}
if err := store.UpdateVolume(tx, volume); err != nil {
logger.WithError(err).Error("failed to update volume")
return nil
}
return nil
})
variable volume may take value nil after extracting data from storage at line
volume := store.GetVolume(tx, volumeID)
If volume is nil, then message about it will be logged and execution of method continues. If I understand it correctly, it may lead to nil ptr dereference when volume.PublishStatus will be referenced in cycle
for _, status := range volume.PublishStatus {
Also I noticed, that in the same processUpdates method there is a call to storage for extracting node but nil as a returned value is processed differently:
for nodeID, nodeUpdate := range nodeUpdates {
err := batch.Update(func(tx store.Tx) error {
logger := logr.WithField("node.id", nodeID)
node := store.GetNode(tx, nodeID)
if node == nil {
logger.Error("node unavailable")
return nil // < --- returning nil
}
I wanted to clarify if there is a mistake in processing nil, returned by store.GetVolume(tx, volumeID) here. And if it is, would it be correct to return nil just like it done with node?
for volumeID, nodes := range unpublishedVolumes {
err := batch.Update(func(tx store.Tx) error {
logger := logr.WithField("volume.id", volumeID)
volume := store.GetVolume(tx, volumeID)
if volume == nil {
logger.Error("volume unavailable")
return nil // <--- proposed change
}
})
Any answer will be appreciated!
Thank you for your time and expertise!
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in manager/dispatcher/dispatcher.go at (*Dispatcher).processUpdates and inspect how missing volumes are handled beside missing nodes. Verify the nil path cannot reach volume.PublishStatus, then run the relevant swarmkit tests; done means a missing volume is logged and processing continues without a nil-pointer dereference.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- distributed-systems
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100