objectstore{,/loose}: Document contracts more clearly

8 files changed, 107 insertions, 33 deletions

diff --git a/objectstore/loose/helpers_test.go b/objectstore/loose/helpers_test.go
index 6cc50163..f4c8dbb3 100644
--- a/objectstore/loose/helpers_test.go
+++ b/objectstore/loose/helpers_test.go
@@ -2,6 +2,7 @@ package loose_test
 
 import (
 	"io"
+	"os"
 	"testing"
 
 	"codeberg.org/lindenii/furgit/internal/testgit"
@@ -65,3 +66,41 @@ func expectedRawObject(t *testing.T, testRepo *testgit.TestRepo, id objectid.Obj
 
 	return ty, body, raw
 }
+
+func corruptLooseObjectTrailer(t *testing.T, testRepo *testgit.TestRepo, id objectid.ObjectID) {
+	t.Helper()
+
+	root := testRepo.OpenObjectsRoot(t)
+
+	hex := id.String()
+	relPath := hex[:2] + "/" + hex[2:]
+
+	file, err := root.OpenFile(relPath, os.O_RDWR, 0)
+	if err != nil {
+		t.Fatalf("OpenFile(%q): %v", relPath, err)
+	}
+
+	defer func() { _ = file.Close() }()
+
+	info, err := file.Stat()
+	if err != nil {
+		t.Fatalf("Stat(%q): %v", relPath, err)
+	}
+
+	if info.Size() == 0 {
+		t.Fatalf("corrupt trailer on empty file %q", relPath)
+	}
+
+	last := make([]byte, 1)
+	_, err = file.ReadAt(last, info.Size()-1)
+	if err != nil {
+		t.Fatalf("ReadAt(%q): %v", relPath, err)
+	}
+
+	last[0] ^= 0xff
+
+	_, err = file.WriteAt(last, info.Size()-1)
+	if err != nil {
+		t.Fatalf("WriteAt(%q): %v", relPath, err)
+	}
+}
diff --git a/objectstore/loose/read_header.go b/objectstore/loose/read_header.go
index 494acfc4..0cd3273d 100644
--- a/objectstore/loose/read_header.go
+++ b/objectstore/loose/read_header.go
@@ -9,6 +9,10 @@ import (
 )
 
 // ReadHeader reads an object's type and declared content length.
+//
+// It parses only enough of the zlib-decoded object to recover the object
+// header. It does not verify that the remaining object content is readable and
+// does not verify the zlib Adler-32 trailer.
 func (store *Store) ReadHeader(id objectid.ObjectID) (objecttype.Type, int64, error) {
 	file, err := store.openObject(id)
 	if err != nil {
diff --git a/objectstore/loose/read_reader.go b/objectstore/loose/read_reader.go
index 035aeaad..e6e130a5 100644
--- a/objectstore/loose/read_reader.go
+++ b/objectstore/loose/read_reader.go
@@ -52,7 +52,13 @@ func (store *Store) openInflated(id objectid.ObjectID) (*os.File, io.ReadCloser,
 }
 
 // ReadReaderFull reads a full serialized object stream as "type size\0content".
+//
 // The caller must close the returned reader.
+//
+// Close releases resources only. It does not drain unread data for additional
+// validation. In particular, malformed trailing compressed data, trailing bytes
+// past the declared object size, and the zlib Adler-32 trailer may go
+// unverified unless the caller reads to io.EOF.
 func (store *Store) ReadReaderFull(id objectid.ObjectID) (io.ReadCloser, error) {
 	file, zr, err := store.openInflated(id)
 	if err != nil {
@@ -79,8 +85,15 @@ func (store *Store) ReadReaderFull(id objectid.ObjectID) (io.ReadCloser, error)
 	}, nil
 }
 
-// ReadReaderContent reads an object's type, declared content length, and content stream.
+// ReadReaderContent reads an object's type, declared content length, and
+// content stream.
+//
 // The caller must close the returned reader.
+//
+// Close releases resources only. It does not drain unread data for additional
+// validation. In particular, malformed trailing compressed data, trailing bytes
+// past the declared object size, and the zlib Adler-32 trailer may go
+// unverified unless the caller reads to io.EOF.
 func (store *Store) ReadReaderContent(id objectid.ObjectID) (objecttype.Type, int64, io.ReadCloser, error) {
 	file, zr, err := store.openInflated(id)
 	if err != nil {
diff --git a/objectstore/loose/read_size.go b/objectstore/loose/read_size.go
index 2a1eaec9..289950d1 100644
--- a/objectstore/loose/read_size.go
+++ b/objectstore/loose/read_size.go
@@ -3,6 +3,9 @@ package loose
 import "codeberg.org/lindenii/furgit/objectid"
 
 // ReadSize reads an object's declared content length.
+//
+// Like ReadHeader, it parses only enough of the zlib-decoded object to recover
+// the header and does not verify the zlib Adler-32 trailer.
 func (store *Store) ReadSize(id objectid.ObjectID) (int64, error) {
 	_, size, err := store.ReadHeader(id)
 
diff --git a/objectstore/loose/read_test.go b/objectstore/loose/read_test.go
index 44e25910..7ecc1b10 100644
--- a/objectstore/loose/read_test.go
+++ b/objectstore/loose/read_test.go
@@ -11,6 +11,7 @@ import (
 	"codeberg.org/lindenii/furgit/objectid"
 	"codeberg.org/lindenii/furgit/objectstore"
 	"codeberg.org/lindenii/furgit/objectstore/loose"
+	"codeberg.org/lindenii/furgit/objecttype"
 )
 
 func TestLooseStoreReadAgainstGit(t *testing.T) {
@@ -174,3 +175,37 @@ func TestLooseStoreNewValidation(t *testing.T) {
 		t.Fatalf("loose.New(root, unknown) expected error")
 	}
 }
+
+func TestLooseStoreReadHeaderDoesNotVerifyAdler32(t *testing.T) {
+	t.Parallel()
+	testgit.ForEachAlgorithm(t, func(t *testing.T, algo objectid.Algorithm) { //nolint:thelper
+		testRepo := testgit.NewRepo(t, testgit.RepoOptions{ObjectFormat: algo, Bare: true})
+		store := openLooseStore(t, testRepo, algo)
+
+		content := []byte("header-only-check\n")
+		id, err := store.WriteBytesContent(objecttype.TypeBlob, content)
+		if err != nil {
+			t.Fatalf("WriteBytesContent: %v", err)
+		}
+
+		corruptLooseObjectTrailer(t, testRepo, id)
+
+		ty, size, err := store.ReadHeader(id)
+		if err != nil {
+			t.Fatalf("ReadHeader: %v", err)
+		}
+
+		if ty != objecttype.TypeBlob {
+			t.Fatalf("ReadHeader type = %v, want %v", ty, objecttype.TypeBlob)
+		}
+
+		if size != int64(len(content)) {
+			t.Fatalf("ReadHeader size = %d, want %d", size, len(content))
+		}
+
+		_, err = store.ReadBytesFull(id)
+		if err == nil {
+			t.Fatalf("ReadBytesFull on corrupted trailer succeeded")
+		}
+	})
+}
diff --git a/objectstore/loose/store.go b/objectstore/loose/store.go
index c3ae989c..11f17594 100644
--- a/objectstore/loose/store.go
+++ b/objectstore/loose/store.go
@@ -9,6 +9,10 @@ import (
 
 // Store reads loose Git objects from an objects directory root.
 //
+// Loose objects are zlib streams whose trailer uses Adler-32. Which reads
+// consume enough of the stream to reach and verify that trailer is documented
+// on the individual methods.
+//
 // Store owns root and closes it in Close.
 type Store struct {
 	// root is the objects directory capability used for all object file access.
@@ -32,6 +36,8 @@ func New(root *os.Root, algo objectid.Algorithm) (*Store, error) {
 }
 
 // Close releases resources associated with the backend.
+//
+// Repeated calls to Close are undefined behavior.
 func (store *Store) Close() error {
 	return store.root.Close()
 }
diff --git a/objectstore/loose/write_writer.go b/objectstore/loose/write_writer.go
index 04a93134..0d6b5b80 100644
--- a/objectstore/loose/write_writer.go
+++ b/objectstore/loose/write_writer.go
@@ -6,7 +6,6 @@ import (
 	"os"
 
 	"codeberg.org/lindenii/furgit/internal/compress/zlib"
-	"codeberg.org/lindenii/furgit/objectid"
 )
 
 const tempObjectFilePrefix = "tmp_obj_"
@@ -38,8 +37,6 @@ type streamWriter struct {
 
 	closed    bool
 	finalized bool
-	finalID   objectid.ObjectID
-	finalErr  error
 }
 
 // newStreamWriter creates a stream writer with a temp file rooted in objects/.
diff --git a/objectstore/loose/write_writer_finalize.go b/objectstore/loose/write_writer_finalize.go
index 0dcae98a..5b7a6754 100644
--- a/objectstore/loose/write_writer_finalize.go
+++ b/objectstore/loose/write_writer_finalize.go
@@ -9,17 +9,14 @@ import (
 )
 
 // Close flushes and closes the underlying zlib stream and temp file.
-// It is safe to call multiple times.
+//
+// Repeated calls to Close are undefined behavior.
 func (writer *streamWriter) Close() error {
-	if writer.closed {
-		return nil
-	}
-
-	writer.closed = true
-
 	errZlib := writer.zw.Close()
 	errSync := writer.file.Sync()
 	errFile := writer.file.Close()
+
+	writer.closed = true
 	writer.file = nil
 
 	return errors.Join(errZlib, errSync, errFile)
@@ -29,10 +26,6 @@ func (writer *streamWriter) Close() error {
 // Publication is no-clobber: it links tmpRelPath to the object path and treats
 // existing destination objects as success.
 func (writer *streamWriter) finalize() (objectid.ObjectID, error) {
-	if writer.finalized {
-		return writer.finalID, writer.finalErr
-	}
-
 	writer.finalized = true
 
 	var zero objectid.ObjectID
@@ -40,37 +33,27 @@ func (writer *streamWriter) finalize() (objectid.ObjectID, error) {
 	if !writer.closed {
 		err := writer.Close()
 		if err != nil {
-			writer.finalErr = err
-
 			return zero, err
 		}
 	}
 
 	if writer.fullMode && !writer.headerDone {
-		writer.finalErr = errors.New("objectstore/loose: missing full object header")
-
-		return zero, writer.finalErr
+		return zero, errors.New("objectstore/loose: missing full object header")
 	}
 
 	if writer.expectedContentLeft != 0 {
-		writer.finalErr = errors.New("objectstore/loose: object content shorter than declared size")
-
-		return zero, writer.finalErr
+		return zero, errors.New("objectstore/loose: object content shorter than declared size")
 	}
 
 	idBytes := writer.hash.Sum(nil)
 
 	id, err := objectid.FromBytes(writer.store.algo, idBytes)
 	if err != nil {
-		writer.finalErr = err
-
 		return zero, err
 	}
 
 	relPath, err := writer.store.objectPath(id)
 	if err != nil {
-		writer.finalErr = err
-
 		return zero, err
 	}
 
@@ -78,8 +61,6 @@ func (writer *streamWriter) finalize() (objectid.ObjectID, error) {
 
 	err = writer.store.root.MkdirAll(dir, 0o755)
 	if err != nil {
-		writer.finalErr = err
-
 		return zero, err
 	}
 
@@ -94,19 +75,15 @@ func (writer *streamWriter) finalize() (objectid.ObjectID, error) {
 	err = writer.store.root.Link(writer.tmpRelPath, relPath)
 	if err != nil {
 		if errors.Is(err, fs.ErrExist) {
-			writer.finalID = id
 			cleanup = false
 			_ = writer.store.root.Remove(writer.tmpRelPath)
 
 			return id, nil
 		}
 
-		writer.finalErr = err
-
 		return zero, err
 	}
 
-	writer.finalID = id
 	cleanup = false
 
 	return id, nil