moby / moby/swarmkit

potential nil ptr dereference in (*Dispatcher).processUpdates (SAST warning)

Open
#3,208 1 comment 0 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.