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

[grey3228]

Hello!

I used SAST tool Svace to analyze swarmkit (master branch, latest commit at the moment is a45be3c) 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!

[grey3228]

@dperny @aaronlehmann @stevvooe @thaJeztah hi, could you please take a look at this issue?