Repository: buchgr/bazel-remote
Pull request #449 (2021-06-23 19:21:38)

Fix empty zstd blob bystestream read infinite loop
If a client tries to download the empty blob via bytestream compressed with zstd, we were leaking a goroutine stuck in a hot infinite loop. rc.Read was returning 0, nil forever, and the the client would timeout.

This change fixes this bug, and adds a test that would hang if the bug wasn't fixed.


Patch:
diff --git a/go.sum b/go.sum
index af8cd2e..d5c1931 100644
--- a/go.sum
+++ b/go.sum
@@ -116,6 +116,7 @@ github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMyw
 github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
 github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.4.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
+github.com/google/go-cmp v0.5.0 h1:/QaMHBdZ26BB3SSst0Iwl10Epc+xhTquomWX0oZEB6w=
 github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs=


Test patch:
diff --git a/server/grpc_bytestream.go b/server/grpc_bytestream.go
index b3a6000..f66a578 100644
--- a/server/grpc_bytestream.go
+++ b/server/grpc_bytestream.go
@@ -32,6 +32,8 @@ var decoderPool = zstdpool.GetDecoderPool()
 
 // ByteStreamServer interface:
 
+var emptyZstdBlob = []byte{40, 181, 47, 253, 32, 0, 1, 0, 0}
+
 func (s *grpcServer) Read(req *bytestream.ReadRequest,
 	resp bytestream.ByteStream_ReadServer) error {
 
@@ -43,6 +45,23 @@ func (s *grpcServer) Read(req *bytestream.ReadRequest,
 		return err
 	}
 
+	if size == 0 {
+		if cmp == casblob.Identity {
+			s.accessLogger.Printf("GRPC BYTESTREAM READ COMPLETED %s", req.ResourceName)
+			return nil
+		}
+
+		// The client asked for a zstd-compressed empty blob. Weird.
+		err := resp.Send(&bytestream.ReadResponse{Data: emptyZstdBlob})
+		if err != nil {
+			msg := fmt.Sprintf("GRPC BYTESTREAM READ FAILED TO SEND RESPONSE: %s %v", hash, err)
+			s.accessLogger.Printf(msg)
+			return status.Error(codes.Unknown, msg)
+		}
+		s.accessLogger.Printf("GRPC BYTESTREAM READ COMPLETED %s", req.ResourceName)
+		return nil
+	}
+
 	if req.ReadOffset < 0 {
 		s.accessLogger.Printf("GRPC BYTESTREAM READ OFFSET INVALID: %s %d",
 			req.ReadOffset)
diff --git a/server/grpc_test.go b/server/grpc_test.go
index 84c0b65..5b188a9 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -609,7 +609,7 @@ func TestGrpcByteStreamEmptySha256(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	var downloadedBlob []byte
+	downloadedBlob := []byte{}
 	for {
 		bsrResp, err := bsrc.Recv()
 		if err == io.EOF {
@@ -628,6 +628,43 @@ func TestGrpcByteStreamEmptySha256(t *testing.T) {
 			t.Fatalf("Downloaded too much data")
 		}
 	}
+
+	// Also test that we can get the "compressed empty blob".
+	// Clients shouldn't do this, but it should be possible.
+
+	resource = fmt.Sprintf("emptyRead/compressed-blobs/zstd/%s/0", emptySha256)
+	bsrReq = bytestream.ReadRequest{ResourceName: resource}
+
+	bsrc, err = bsClient.Read(ctx, &bsrReq)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	downloadedBlob = []byte{}
+	for {
+		bsrResp, err := bsrc.Recv()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			t.Fatal(err)
+		}
+		if bsrResp == nil {
+			t.Fatalf("Expected non-nil response")
+		}
+
+		downloadedBlob = append(downloadedBlob, bsrResp.Data...)
+
+		if len(downloadedBlob) > len(emptyZstdBlob) {
+			t.Fatalf("Downloaded too much data")
+		}
+	}
+
+	if !bytes.Equal(downloadedBlob, emptyZstdBlob) {
+		// There are many different valid empty zstd blob representations,
+		// but we picked this one.
+		t.Fatalf("Expected compressed empty blob to be available")
+	}
 }
 
 func TestGrpcByteStream(t *testing.T) {