diff options
| -rw-r--r-- | objectstore/loose/helpers_test.go | 39 | ||||
| -rw-r--r-- | objectstore/loose/read_header.go | 4 | ||||
| -rw-r--r-- | objectstore/loose/read_reader.go | 15 | ||||
| -rw-r--r-- | objectstore/loose/read_size.go | 3 | ||||
| -rw-r--r-- | objectstore/loose/read_test.go | 35 | ||||
| -rw-r--r-- | objectstore/loose/store.go | 6 | ||||
| -rw-r--r-- | objectstore/loose/write_writer.go | 3 | ||||
| -rw-r--r-- | objectstore/loose/write_writer_finalize.go | 35 | ||||
| -rw-r--r-- | objectstore/objectstore.go | 23 |
9 files changed, 130 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 diff --git a/objectstore/objectstore.go b/objectstore/objectstore.go index 2592c1b1..f152a7f0 100644 --- a/objectstore/objectstore.go +++ b/objectstore/objectstore.go @@ -25,27 +25,47 @@ type Store interface { // In a valid repository, hashing this payload with the same algorithm yields // the requested object ID. Readers should treat this as a repository // invariant and should not re-verify it on every read. + // + // Any read-time integrity verification beyond producing this payload is + // implementation-defined. ReadBytesFull(id objectid.ObjectID) ([]byte, error) // ReadBytesContent reads an object's type and content bytes. + // + // Any read-time integrity verification beyond producing this payload is + // implementation-defined. ReadBytesContent(id objectid.ObjectID) (objecttype.Type, []byte, error) // ReadReaderFull reads a full serialized object stream as "type size\0content". + // // Caller must close the returned reader. + // + // Any read-time integrity verification performed while producing the stream + // is implementation-defined. ReadReaderFull(id objectid.ObjectID) (io.ReadCloser, error) // ReadReaderContent reads an object's type, declared content length, // and content stream. + // // Caller must close the returned reader. + // + // Any read-time integrity verification performed while producing the stream + // is implementation-defined. ReadReaderContent(id objectid.ObjectID) (objecttype.Type, int64, io.ReadCloser, error) // ReadSize reads an object's declared content length. // // This is equivalent to ReadHeader(...).size and may be cheaper than // ReadHeader when callers do not need object type. + // + // Any read-time integrity verification performed to produce the size is + // implementation-defined. ReadSize(id objectid.ObjectID) (int64, error) // ReadHeader reads an object's type and declared content length. + // + // Any read-time integrity verification performed to produce the header is + // implementation-defined. ReadHeader(id objectid.ObjectID) (objecttype.Type, int64, error) // Refresh updates any backend-local discovery/cache view of on-disk objects. @@ -54,6 +74,9 @@ type Store interface { Refresh() error // Close releases resources associated with the backend. + // + // Repeated calls to Close are undefined behavior unless the implementation + // explicitly documents otherwise. Close() error } |