Don’t silently ignore errors when decoding JSON or deleting incomplete layers

containers.go

@@ -281,26 +281,30 @@ func (r *containerStore) load(lockedForWriting bool) error {
 	if err != nil && !os.IsNotExist(err) {
 		return err
 	}
+
 	containers := []*Container{}
+	if len(data) != 0 {
+		if err := json.Unmarshal(data, &containers); err != nil {
+			return fmt.Errorf("loading %q: %w", rpath, err)
+		}
+	}
+	idlist := make([]string, 0, len(containers))
 	layers := make(map[string]*Container)
-	idlist := []string{}
 	ids := make(map[string]*Container)
 	names := make(map[string]*Container)
-	if err = json.Unmarshal(data, &containers); len(data) == 0 || err == nil {
-		idlist = make([]string, 0, len(containers))
-		for n, container := range containers {
-			idlist = append(idlist, container.ID)
-			ids[container.ID] = containers[n]
-			layers[container.LayerID] = containers[n]
-			for _, name := range container.Names {
-				if conflict, ok := names[name]; ok {
-					r.removeName(conflict, name)
-					needSave = true
-				}
-				names[name] = containers[n]
+	for n, container := range containers {
+		idlist = append(idlist, container.ID)
+		ids[container.ID] = containers[n]
+		layers[container.LayerID] = containers[n]
+		for _, name := range container.Names {
+			if conflict, ok := names[name]; ok {
+				r.removeName(conflict, name)
+				needSave = true
 			}
+			names[name] = containers[n]
 		}
 	}
+
 	r.containers = containers
 	r.idindex = truncindex.NewTruncIndex(idlist) // Invalid values in idlist are ignored: they are not a reason to refuse processing the whole store.
 	r.byid = ids
@@ -426,33 +430,31 @@ func (r *containerStore) Create(id string, names []string, image, layer, metadat
 	if err := hasOverlappingRanges(options.GIDMap); err != nil {
 		return nil, err
 	}
-	if err == nil {
-		container = &Container{
-			ID:             id,
-			Names:          names,
-			ImageID:        image,
-			LayerID:        layer,
-			Metadata:       metadata,
-			BigDataNames:   []string{},
-			BigDataSizes:   make(map[string]int64),
-			BigDataDigests: make(map[string]digest.Digest),
-			Created:        time.Now().UTC(),
-			Flags:          copyStringInterfaceMap(options.Flags),
-			UIDMap:         copyIDMap(options.UIDMap),
-			GIDMap:         copyIDMap(options.GIDMap),
-		}
-		r.containers = append(r.containers, container)
-		r.byid[id] = container
-		// This can only fail on duplicate IDs, which shouldn’t happen — and in that case the index is already in the desired state anyway.
-		// Implementing recovery from an unlikely and unimportant failure here would be too risky.
-		_ = r.idindex.Add(id)
-		r.bylayer[layer] = container
-		for _, name := range names {
-			r.byname[name] = container
-		}
-		err = r.Save()
-		container = copyContainer(container)
+	container = &Container{
+		ID:             id,
+		Names:          names,
+		ImageID:        image,
+		LayerID:        layer,
+		Metadata:       metadata,
+		BigDataNames:   []string{},
+		BigDataSizes:   make(map[string]int64),
+		BigDataDigests: make(map[string]digest.Digest),
+		Created:        time.Now().UTC(),
+		Flags:          copyStringInterfaceMap(options.Flags),
+		UIDMap:         copyIDMap(options.UIDMap),
+		GIDMap:         copyIDMap(options.GIDMap),
+	}
+	r.containers = append(r.containers, container)
+	r.byid[id] = container
+	// This can only fail on duplicate IDs, which shouldn’t happen — and in that case the index is already in the desired state anyway.
+	// Implementing recovery from an unlikely and unimportant failure here would be too risky.
+	_ = r.idindex.Add(id)
+	r.bylayer[layer] = container
+	for _, name := range names {
+		r.byname[name] = container
 	}
+	err = r.Save()
+	container = copyContainer(container)
 	return container, err
 }
 

images.go

@@ -349,37 +349,40 @@ func (r *imageStore) load(lockedForWriting bool) error {
 	if err != nil && !os.IsNotExist(err) {
 		return err
 	}
+
 	images := []*Image{}
-	idlist := []string{}
+	if len(data) != 0 {
+		if err := json.Unmarshal(data, &images); err != nil {
+			return fmt.Errorf("loading %q: %w", rpath, err)
+		}
+	}
+	idlist := make([]string, 0, len(images))
 	ids := make(map[string]*Image)
 	names := make(map[string]*Image)
 	digests := make(map[digest.Digest][]*Image)
-	if err = json.Unmarshal(data, &images); len(data) == 0 || err == nil {
-		idlist = make([]string, 0, len(images))
-		for n, image := range images {
-			ids[image.ID] = images[n]
-			idlist = append(idlist, image.ID)
-			for _, name := range image.Names {
-				if conflict, ok := names[name]; ok {
-					r.removeName(conflict, name)
-					shouldSave = true
-				}
-			}
-			// Compute the digest list.
-			err = image.recomputeDigests()
-			if err != nil {
-				return fmt.Errorf("computing digests for image with ID %q (%v): %w", image.ID, image.Names, err)
-			}
-			for _, name := range image.Names {
-				names[name] = image
+	for n, image := range images {
+		ids[image.ID] = images[n]
+		idlist = append(idlist, image.ID)
+		for _, name := range image.Names {
+			if conflict, ok := names[name]; ok {
+				r.removeName(conflict, name)
+				shouldSave = true
 			}
-			for _, digest := range image.Digests {
-				list := digests[digest]
-				digests[digest] = append(list, image)
-			}
-			image.ReadOnly = !r.lockfile.IsReadWrite()
 		}
+		// Compute the digest list.
+		if err := image.recomputeDigests(); err != nil {
+			return fmt.Errorf("computing digests for image with ID %q (%v): %w", image.ID, image.Names, err)
+		}
+		for _, name := range image.Names {
+			names[name] = image
+		}
+		for _, digest := range image.Digests {
+			list := digests[digest]
+			digests[digest] = append(list, image)
+		}
+		image.ReadOnly = !r.lockfile.IsReadWrite()
 	}
+
 	if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
 		// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
 		return ErrDuplicateImageNames

layers.go

@@ -470,40 +470,43 @@ func (r *layerStore) load(lockedForWriting bool) error {
 	if err != nil && !os.IsNotExist(err) {
 		return err
 	}
+
 	layers := []*Layer{}
-	idlist := []string{}
+	if len(data) != 0 {
+		if err := json.Unmarshal(data, &layers); err != nil {
+			return fmt.Errorf("loading %q: %w", rpath, err)
+		}
+	}
+	idlist := make([]string, 0, len(layers))
 	ids := make(map[string]*Layer)
 	names := make(map[string]*Layer)
 	compressedsums := make(map[digest.Digest][]string)
 	uncompressedsums := make(map[digest.Digest][]string)
 	if r.lockfile.IsReadWrite() {
 		selinux.ClearLabels()
 	}
-	if err = json.Unmarshal(data, &layers); len(data) == 0 || err == nil {
-		idlist = make([]string, 0, len(layers))
-		for n, layer := range layers {
-			ids[layer.ID] = layers[n]
-			idlist = append(idlist, layer.ID)
-			for _, name := range layer.Names {
-				if conflict, ok := names[name]; ok {
-					r.removeName(conflict, name)
-					shouldSave = true
-				}
-				names[name] = layers[n]
-			}
-			if layer.CompressedDigest != "" {
-				compressedsums[layer.CompressedDigest] = append(compressedsums[layer.CompressedDigest], layer.ID)
-			}
-			if layer.UncompressedDigest != "" {
-				uncompressedsums[layer.UncompressedDigest] = append(uncompressedsums[layer.UncompressedDigest], layer.ID)
+	for n, layer := range layers {
+		ids[layer.ID] = layers[n]
+		idlist = append(idlist, layer.ID)
+		for _, name := range layer.Names {
+			if conflict, ok := names[name]; ok {
+				r.removeName(conflict, name)
+				shouldSave = true
 			}
-			if layer.MountLabel != "" {
-				selinux.ReserveLabel(layer.MountLabel)
-			}
-			layer.ReadOnly = !r.lockfile.IsReadWrite()
+			names[name] = layers[n]
 		}
-		err = nil
+		if layer.CompressedDigest != "" {
+			compressedsums[layer.CompressedDigest] = append(compressedsums[layer.CompressedDigest], layer.ID)
+		}
+		if layer.UncompressedDigest != "" {
+			uncompressedsums[layer.UncompressedDigest] = append(uncompressedsums[layer.UncompressedDigest], layer.ID)
+		}
+		if layer.MountLabel != "" {
+			selinux.ReserveLabel(layer.MountLabel)
+		}
+		layer.ReadOnly = !r.lockfile.IsReadWrite()
 	}
+
 	if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
 		// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
 		return ErrDuplicateLayerNames
@@ -519,13 +522,14 @@ func (r *layerStore) load(lockedForWriting bool) error {
 	if r.lockfile.IsReadWrite() {
 		r.mountsLockfile.RLock()
 		defer r.mountsLockfile.Unlock()
-		if err = r.loadMounts(); err != nil {
+		if err := r.loadMounts(); err != nil {
 			return err
 		}
 
 		// Last step: as we’re writable, try to remove anything that a previous
 		// user of this storage area marked for deletion but didn't manage to
 		// actually delete.
+		var incompleteDeletionErrors error // = nil
 		if lockedForWriting {
 			for _, layer := range r.layers {
 				if layer.Flags == nil {
@@ -535,18 +539,26 @@ func (r *layerStore) load(lockedForWriting bool) error {
 					logrus.Warnf("Found incomplete layer %#v, deleting it", layer.ID)
 					err = r.deleteInternal(layer.ID)
 					if err != nil {
-						break
+						// Don't return the error immediately, because deleteInternal does not saveLayers();
+						// Even if deleting one incomplete layer fails, call saveLayers() so that other possible successfully
+						// deleted incomplete layers have their metadata correctly removed.
+						incompleteDeletionErrors = multierror.Append(incompleteDeletionErrors,
+							fmt.Errorf("deleting layer %#v: %w", layer.ID, err))
 					}
 					shouldSave = true
 				}
 			}
 		}
 		if shouldSave {
-			return r.saveLayers()
+			if err := r.saveLayers(); err != nil {
+				return err
+			}
+		}
+		if incompleteDeletionErrors != nil {
+			return incompleteDeletionErrors
 		}
 	}
-
-	return err
+	return nil
 }
 
 func (r *layerStore) loadMounts() error {
@@ -1836,13 +1848,11 @@ func (r *layerStore) applyDiffWithOptions(to string, layerOptions *LayerOptions,
 		return -1, err
 	}
 	compressor.Close()
-	if err == nil {
-		if err := os.MkdirAll(filepath.Dir(r.tspath(layer.ID)), 0700); err != nil {
-			return -1, err
-		}
-		if err := ioutils.AtomicWriteFile(r.tspath(layer.ID), tsdata.Bytes(), 0600); err != nil {
-			return -1, err
-		}
+	if err := os.MkdirAll(filepath.Dir(r.tspath(layer.ID)), 0700); err != nil {
+		return -1, err
+	}
+	if err := ioutils.AtomicWriteFile(r.tspath(layer.ID), tsdata.Bytes(), 0600); err != nil {
+		return -1, err
 	}
 	if compressedDigester != nil {
 		compressedDigest = compressedDigester.Digest()