contenthash: improve the correctness of needsScan

Commit f724d6f ("contenthash: implement proper Linux symlink
semantics for needsScan") fixed issues with needScan's handling of
symlinks, but the logic used to figure out if a parent path is in the
cache was incorrect in two ways:

 1. The optimisations in getFollowSymlinksCallback to avoid looking up /
    or no-op components in the cache lead to the callback not being
    called when we go through / (both in for the initial currentPath=/
    state and for absolute symlinks). The upshot is that
    needsScan(/non-existent-path) would always return true because we
    didn't check if / has been scanned.

    There were also some issues with the wrong cache record being
    returned if you hit a symlink to only /. These optimisations only
    make sense if are only returning a path (as in rootPath) and not
    anything else.

 2. Because needsScan would only store the _last_ good path, cases with
    symlink jumps to non-existent paths within directories already
    scanned would result in a re-scan that isn't necessary. Fix this by
    saving a set of prefix paths we have seen. This change also means
    that we can also simplify the logic in the needsScan callback.

    Note that in combination with (1) and (2), if / has been scanned
    then needsScan will always return false now (because the / prefix is
    always checked against, and every path has it as a parent).

    The pathSet structure is the "dumb" way of doing it. We could use a
    radix tree and LongestPrefix() to implement it in a smarter way, but
    in practice the list will be very small and is both short-lived and
    write-many-read-few, so go-immutable-radix's node copy cost probably
    makes the array better in practice anyway.

Fixes: f724d6f ("contenthash: implement proper Linux symlink semantics for needsScan")
Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

cache/contenthash/checksum.go

@@ -928,31 +928,64 @@ func (cc *cacheContext) checksum(ctx context.Context, root *iradix.Node[*CacheRe
 	return cr2, true, nil
 }
 
+// pathSet is a set of path prefixes that can be used to see if a given path is
+// lexically a child of any path in the set. All paths provided to this set
+// MUST be absolute and use / as the separator.
+type pathSet struct {
+	// prefixes contains paths of the form "/a/b/", so that we correctly detect
+	// /a/b as being a parent of /a/b/c but not /a/bc.
+	prefixes []string
+}
+
+// add a path to the set. This is a no-op if includes(path) == true.
+func (s *pathSet) add(p string) {
+	// Ensure the path is absolute and clean.
+	p = path.Join("/", p)
+	if !s.includes(p) {
+		if p != "/" {
+			p += "/"
+		}
+		s.prefixes = append(s.prefixes, p)
+	}
+}
+
+// includes returns true iff there is a path in the pathSet which is a lexical
+// parent of the given path. The provided path MUST be an absolute path and
+// MUST NOT contain any ".." components, as they will be path.Clean'd.
+func (s pathSet) includes(p string) bool {
+	// Ensure the path is absolute and clean.
+	p = path.Join("/", p)
+	if p != "/" {
+		p += "/"
+	}
+	for _, prefix := range s.prefixes {
+		if strings.HasPrefix(p, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 // needsScan returns false if path is in the tree or a parent path is in tree
 // and subpath is missing.
 func (cc *cacheContext) needsScan(root *iradix.Node[*CacheRecord], path string, followTrailing bool) (bool, error) {
 	var (
-		lastGoodPath    string
+		goodPaths       pathSet
 		hasParentInTree bool
 	)
 	k := convertPathToKey(path)
 	_, cr, err := getFollowLinksCallback(root, k, followTrailing, func(subpath string, cr *CacheRecord) error {
+		// If we found a path that exists in the cache, add it to the set of
+		// known-scanned paths. Otherwise, verify whether the not-found subpath
+		// is inside a known-scanned path (we might have hit a "..", taking us
+		// out of the scanned paths, or we might hit a non-existent path inside
+		// a scanned path). getFollowLinksCallback iterates left-to-right, so
+		// we will always hit ancestors first.
 		if cr != nil {
-			// If the path is not a symlink, then for now we have a parent in
-			// the tree. Otherwise, we reset hasParentInTree because we
-			// might've jumped to a part of the tree that hasn't been scanned.
-			hasParentInTree = (cr.Type != CacheRecordTypeSymlink)
-			if hasParentInTree {
-				lastGoodPath = subpath
-			}
-		} else if hasParentInTree {
-			// If the current component was something like ".." and the subpath
-			// couldn't be found, then we need to invalidate hasParentInTree.
-			// In practice this means that our subpath needs to be prefixed by
-			// the last good path. We add a trailing slash to make sure the
-			// prefix is a proper lexical prefix (as opposed to /a/b being seen
-			// as a prefix of /a/bc).
-			hasParentInTree = strings.HasPrefix(subpath+"/", lastGoodPath+"/")
+			hasParentInTree = cr.Type != CacheRecordTypeSymlink
+			goodPaths.add(subpath)
+		} else {
+			hasParentInTree = goodPaths.includes(subpath)
 		}
 		return nil
 	})
@@ -1005,11 +1038,7 @@ func (cc *cacheContext) scanPath(ctx context.Context, m *mount, p string, follow
 		if err != nil {
 			return err
 		}
-		p := path.Join("/", filepath.ToSlash(rel))
-		if p == "/" {
-			p = ""
-		}
-		k := convertPathToKey(p)
+		k := convertPathToKey(keyPath(rel))
 		if _, ok := n.Get(k); !ok {
 			cr := &CacheRecord{
 				Type: CacheRecordTypeFile,
@@ -1060,9 +1089,18 @@ func getFollowLinks(root *iradix.Node[*CacheRecord], k []byte, followTrailing bo
 // Linux APIs), followTrailing is obeyed even if the key has a trailing slash
 // (though paths like "foo/link/." will cause the link to be resolved).
 //
-// The callback cb is called after each cache lookup done by
-// getFollowLinksCallback, except for the first lookup where the verbatim key
-// is looked up in the cache.
+// cb is a callback that is called for each path component encountered during
+// path resolution (after the path component is looked up in the cache). This
+// means for a path like /a/b/c, the callback will be called for at least
+//
+//	{/, /a, /a/b, /a/b/c}
+//
+// Note that if any of the components are symlinks, the paths will depend on
+// the symlink contents and there will be more callbacks. If the requested key
+// has a trailing slash, the callback will also be called for the final
+// trailing-slash lookup (/a/b/c/ in the above example). Note that
+// getFollowLinksCallback will try to look up the original key directly first
+// and the callback is not called for this first lookup.
 func getFollowLinksCallback(root *iradix.Node[*CacheRecord], k []byte, followTrailing bool, cb followLinksCallback) ([]byte, *CacheRecord, error) {
 	v, ok := root.Get(k)
 	if ok && (!followTrailing || v.Type != CacheRecordTypeSymlink) {
@@ -1093,16 +1131,11 @@ func getFollowLinksCallback(root *iradix.Node[*CacheRecord], k []byte, followTra
 		// Apply the component to the path. Since it is a single component, and
 		// our current path contains no symlinks, we can just apply it
 		// leixically.
-		nextPath := path.Join("/", currentPath, part)
-		if nextPath == "/" {
-			// If we hit the root, just treat it as a directory.
-			currentPath = "/"
-			continue
-		}
-		if nextPath == currentPath {
-			// noop component
-			continue
-		}
+		nextPath := keyPath(path.Join("/", currentPath, part))
+		// In contrast to rootPath, we don't skip lookups for no-op components
+		// or / because we need to call the callback for every path component
+		// we hit (including /) and we need to make sure that the CacheRecord
+		// we return is correct after every iteration.
 
 		cr, ok = root.Get(convertPathToKey(nextPath))
 		if cb != nil {