Repository: buchgr/bazel-remote
Pull request #168 (2020-01-27 05:52:10)

disk cache: check that keys are the correct length
The key format is checked properly in the http and grpc server code, this
simple check should help catch incorrect internal usage (eg test cases).


Patch:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index 9355eeb..5c40eb3 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -53,6 +53,8 @@ type DiskCache struct {
 	lru SizedLRU
 }
 
+const sha256HashStrSize = sha256.Size * 2 // Two hex characters per byte.
+
 // New returns a new instance of a filesystem-based cache rooted at `dir`,
 // with a maximum size of `maxSizeBytes` bytes and an optional backend `proxy`.
 // DiskCache is safe for concurrent use.
@@ -181,10 +183,18 @@ func (c *DiskCache) loadExistingFiles() error {
 // If `hash` is not the empty string, and the contents don't match it,
 // a non-nil error is returned.
 func (c *DiskCache) Put(kind cache.EntryKind, hash string, expectedSize int64, r io.Reader) error {
-	c.mu.Lock()
+
+	// The hash format is checked properly in the http/grpc code.
+	// Just perform a simple/fast check here, to catch bad tests.
+	if len(hash) != sha256HashStrSize {
+		return fmt.Errorf("Invalid hash size: %d, expected: %d",
+			len(hash), sha256.Size)
+	}
 
 	key := cacheKey(kind, hash)
 
+	c.mu.Lock()
+
 	// If there's an ongoing upload (i.e. cache key is present in uncommitted state),
 	// we drop the upload and discard the incoming stream. We do accept uploads
 	// of existing keys, as it should happen relatively rarely (e.g. race
@@ -326,6 +336,13 @@ func (c *DiskCache) availableOrTryProxy(key string) (bool, *lruItem) {
 // it is returned.
 func (c *DiskCache) Get(kind cache.EntryKind, hash string) (io.ReadCloser, int64, error) {
 
+	// The hash format is checked properly in the http/grpc code.
+	// Just perform a simple/fast check here, to catch bad tests.
+	if len(hash) != sha256HashStrSize {
+		return nil, -1, fmt.Errorf("Invalid hash size: %d, expected: %d",
+			len(hash), sha256.Size)
+	}
+
 	var err error
 	key := cacheKey(kind, hash)
 
@@ -432,6 +449,13 @@ func (c *DiskCache) Get(kind cache.EntryKind, hash string) (io.ReadCloser, int64
 // If there is a local cache miss, the proxy backend (if there is
 // one) will be checked.
 func (c *DiskCache) Contains(kind cache.EntryKind, hash string) bool {
+
+	// The hash format is checked properly in the http/grpc code.
+	// Just perform a simple/fast check here, to catch bad tests.
+	if len(hash) != sha256HashStrSize {
+		return false
+	}
+
 	c.mu.Lock()
 	val, found := c.lru.Get(cacheKey(kind, hash))
 	// Uncommitted (i.e. uploading items) should be reported as not ok


Test patch:
diff --git a/cache/disk/disk_test.go b/cache/disk/disk_test.go
index 0bf22a0..8e83bf4 100644
--- a/cache/disk/disk_test.go
+++ b/cache/disk/disk_test.go
@@ -129,7 +129,15 @@ func TestCacheEviction(t *testing.T) {
 
 	for i, thisExp := range expectedSizesNumItems {
 		strReader := strings.NewReader(strings.Repeat("a", i))
-		err := testCache.Put(cache.AC, fmt.Sprintf("aa-%d", i), int64(i), strReader)
+
+		// Suitably-sized, unique key for these testcases:
+		key := fmt.Sprintf("%0*d", sha256HashStrSize, i)
+		if len(key) != sha256.Size*2 {
+			t.Fatalf("invalid testcase- key length should be %d, not %d: %s",
+				sha256.Size*2, len(key), key)
+		}
+
+		err := testCache.Put(cache.AC, key, int64(i), strReader)
 		if err != nil {
 			t.Fatal(err)
 		}