diff --git a/internal/gitengine/delta.go b/internal/gitengine/delta.go index 69d0598..47c954d 100644 --- a/internal/gitengine/delta.go +++ b/internal/gitengine/delta.go @@ -103,8 +103,11 @@ func ApplyDelta(base, delta []byte) ([]byte, error) { return applyDeltaInstructions(target, base, delta[pos:], targetSize) } -// ApplyDeltaWithBuffer decodes the delta into a caller-supplied or pooled buffer. -func ApplyDeltaWithBuffer(targetBuf []byte, base, delta []byte) ([]byte, error) { +// ApplyDeltaWithBuffer decodes the delta into the buffer pointed to by targetBuf, +// which must be non-nil. When the decoded target does not fit, a larger array is +// allocated and written back through targetBuf so that a caller recycling a +// pooled buffer keeps the grown array instead of the outgrown one. +func ApplyDeltaWithBuffer(targetBuf *[]byte, base, delta []byte) ([]byte, error) { baseSize, targetSize, pos, err := ReadDeltaHeader(delta) if err != nil { return nil, err @@ -114,13 +117,18 @@ func ApplyDeltaWithBuffer(targetBuf []byte, base, delta []byte) ([]byte, error) return nil, fmt.Errorf("%w: expected base size %d, got %d", ErrDeltaBaseMismatch, baseSize, len(base)) } - if cap(targetBuf) < targetSize { - targetBuf = make([]byte, 0, targetSize) + if cap(*targetBuf) < targetSize { + *targetBuf = make([]byte, 0, targetSize) } else { - targetBuf = targetBuf[:0] + *targetBuf = (*targetBuf)[:0] } - return applyDeltaInstructions(targetBuf, base, delta[pos:], targetSize) + target, err := applyDeltaInstructions(*targetBuf, base, delta[pos:], targetSize) + if err != nil { + return nil, err + } + *targetBuf = target + return target, nil } func applyDeltaInstructions(target []byte, base, instructions []byte, targetSize int) ([]byte, error) { diff --git a/internal/gitengine/delta_test.go b/internal/gitengine/delta_test.go index ab40ec0..4c4870e 100644 --- a/internal/gitengine/delta_test.go +++ b/internal/gitengine/delta_test.go @@ -70,7 +70,7 @@ func TestApplyDelta(t *testing.T) { // Test 3: Pooled buffer decoding buf := make([]byte, 0, 256) - res3, err := ApplyDeltaWithBuffer(buf, base, delta2) + res3, err := ApplyDeltaWithBuffer(&buf, base, delta2) if err != nil { t.Fatalf("ApplyDeltaWithBuffer failed: %v", err) } diff --git a/internal/gitengine/hardening_test.go b/internal/gitengine/hardening_test.go index a2555f3..98a0308 100644 --- a/internal/gitengine/hardening_test.go +++ b/internal/gitengine/hardening_test.go @@ -3,6 +3,7 @@ package gitengine import ( "bytes" "encoding/binary" + "encoding/hex" "errors" "fmt" "os" @@ -193,32 +194,103 @@ func TestSafety_Walker_DeepLinearHistory(t *testing.T) { } } -// SEC-15: Concurrency safety in RepositoryReader (concurrent ReadObject and Close) +// SEC-15: Concurrency safety in RepositoryReader (concurrent ReadObject and Close). +// +// Close closes the *os.File behind every PackReader, so a read that is already in +// flight must not be handed a closed descriptor: ReadAt would fail with +// os.ErrFileClosed, which the reader used to swallow and report as +// ErrObjectNotFound — a silent wrong answer that -race cannot see. Every read here +// must therefore either succeed with the real payload or fail with ErrReaderClosed. func TestConcurrency_RepositoryReader_Deadlock(t *testing.T) { - reader := &RepositoryReader{} + content := []byte("payload read concurrently with Close") + obj, sha := blobObject(content) + gitDir := writeTestPack(t, "concurrent", []testPackObj{obj}) + oid := hex.EncodeToString(sha[:]) + + reader, err := NewRepositoryReader(&RepoInfo{CommonGitDir: gitDir}) + if err != nil { + t.Fatalf("NewRepositoryReader failed: %v", err) + } + defer func() { _ = reader.Close() }() var wg sync.WaitGroup - // Run concurrent ReadObject and Close calls - for i := 0; i < 20; i++ { + start := make(chan struct{}) + + for range 20 { wg.Add(1) go func() { defer wg.Done() - for j := 0; j < 100; j++ { - _, _ = reader.ReadObject("0123456789abcdef0123456789abcdef01234567") - _ = reader.HasObject("0123456789abcdef0123456789abcdef01234567") + <-start + for range 100 { + got, err := reader.ReadObject(oid) + switch { + case err == nil: + if !bytes.Equal(got.Data, content) { + t.Errorf("ReadObject returned wrong payload %q", got.Data) + } + case errors.Is(err, ErrObjectNotFound): + t.Errorf("a packed object was reported missing during shutdown: %v", err) + case !errors.Is(err, ErrReaderClosed): + t.Errorf("expected ErrReaderClosed, got %v", err) + } + // Exercised for lock coverage: a closed reader reports absence. + _ = reader.HasObject(oid) } }() } - for i := 0; i < 5; i++ { + for range 5 { wg.Add(1) go func() { defer wg.Done() - _ = reader.Close() + <-start + if err := reader.Close(); err != nil { + t.Errorf("Close failed: %v", err) + } }() } + close(start) wg.Wait() + + // After Close, reads report the reader as closed rather than the object missing. + if _, err := reader.ReadObject(oid); !errors.Is(err, ErrReaderClosed) { + t.Fatalf("expected ErrReaderClosed after Close, got %v", err) + } + if reader.HasObject(oid) { + t.Error("HasObject should report false after Close") + } +} + +// PERF-05: a delta target that outgrows the pooled buffer must be written back +// through the caller's pointer, otherwise the caller recycles the small buffer +// forever and the pool is defeated on exactly the objects it exists for. +func TestPerformance_ApplyDeltaWithBuffer_GrowsCallerBuffer(t *testing.T) { + base := bytes.Repeat([]byte("g"), 4096) + targetSize := len(base) + 3 + + var delta []byte + delta = append(delta, encodeLEB128(len(base))...) + delta = append(delta, encodeLEB128(targetSize)...) + // Copy all of base: 2 offset bytes (0) and 2 size bytes (4096). + delta = append(delta, 0x80|0x01|0x02|0x10|0x20, 0x00, 0x00, 0x00, 0x10) + // Insert three literal bytes. + delta = append(delta, 3, 'e', 'n', 'd') + + buf := make([]byte, 0, 8) + target, err := ApplyDeltaWithBuffer(&buf, base, delta) + if err != nil { + t.Fatalf("ApplyDeltaWithBuffer failed: %v", err) + } + if len(target) != targetSize { + t.Fatalf("expected target size %d, got %d", targetSize, len(target)) + } + if cap(buf) < targetSize { + t.Fatalf("caller buffer was not grown: cap %d, need %d", cap(buf), targetSize) + } + if !bytes.Equal(buf, target) { + t.Fatalf("caller buffer does not hold the decoded target: len %d vs %d", len(buf), len(target)) + } } // PERF-04: CompareTreeEntries canonical ordering verification diff --git a/internal/gitengine/loose.go b/internal/gitengine/loose.go index a012375..8ee4392 100644 --- a/internal/gitengine/loose.go +++ b/internal/gitengine/loose.go @@ -13,7 +13,36 @@ import ( "github.com/klauspost/compress/zlib" ) +// resettableZlibReader is a pooled zlib reader that can be rebound to a new source. +type resettableZlibReader interface { + io.ReadCloser + zlib.Resetter +} + +// newPooledZlibReader binds a reader from pool to src, falling back to a fresh +// zlib reader when the pool is empty. +// +// A failed Reset is terminal for src: zlib.Reset consumes the two-byte RFC-1950 +// header eagerly and installs its own bufio.Reader with up to 4 KiB of read-ahead +// before it can report an error, so src is no longer positioned at the start of +// the stream. Retrying with zlib.NewReader(src) would parse from a mutated offset +// and report a misleading error about genuinely corrupt input, so the Reset error +// is returned as-is instead. +func newPooledZlibReader(pool *sync.Pool, src io.Reader) (io.ReadCloser, error) { + if pooled, ok := pool.Get().(resettableZlibReader); ok { + if err := pooled.Reset(src, nil); err != nil { + return nil, err + } + return pooled, nil + } + return zlib.NewReader(src) +} + // LooseReader provides read access to loose Git objects located in .git/objects/??/* +// +// Concurrency: safe for concurrent use by multiple goroutines. It holds no open +// file handles between calls — each read opens and closes its own file — and its +// only mutable state is the zlib reader pool. type LooseReader struct { objectsDir string zlibPool sync.Pool @@ -66,23 +95,9 @@ func (r *LooseReader) ReadObject(oid string) (*Object, error) { } defer f.Close() - var zReader io.ReadCloser - if pooled := r.zlibPool.Get(); pooled != nil { - if zr, ok := pooled.(zlib.Resetter); ok { - if resetErr := zr.Reset(f, nil); resetErr == nil { - if rc, ok := pooled.(io.ReadCloser); ok { - zReader = rc - } - } - } - } - - if zReader == nil { - var zErr error - zReader, zErr = zlib.NewReader(f) - if zErr != nil { - return nil, fmt.Errorf("%w: failed to create zlib reader for %s: %v", ErrCorruptObject, oid, zErr) - } + zReader, err := newPooledZlibReader(&r.zlibPool, f) + if err != nil { + return nil, fmt.Errorf("%w: failed to create zlib reader for %s: %v", ErrCorruptObject, oid, err) } defer func() { _ = zReader.Close() diff --git a/internal/gitengine/object.go b/internal/gitengine/object.go index 0aaefe8..ae492f7 100644 --- a/internal/gitengine/object.go +++ b/internal/gitengine/object.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "sync" + "sync/atomic" "github.com/kryft-dev/grg/internal/model" ) @@ -17,6 +18,11 @@ var ErrObjectNotFound = errors.New("git object not found") // ErrCorruptObject indicates an unreadable or corrupt Git object. var ErrCorruptObject = errors.New("corrupt git object") +// ErrReaderClosed indicates that the repository reader was closed, either before +// the read started or while it was in flight. It is deliberately distinct from +// ErrObjectNotFound so that a shutdown race is never reported as a missing object. +var ErrReaderClosed = errors.New("repository reader closed") + // ObjectType represents the type of a Git object. type ObjectType uint8 @@ -82,10 +88,20 @@ type ObjectReader interface { } // RepositoryReader combines loose object storage and packfiles into a unified ObjectReader. +// +// Concurrency: ReadObject and HasObject are safe for concurrent use, and may be +// re-entered from inside a read when a packfile resolves an OBJ_REF_DELTA base +// through this reader. mu therefore guards only the loose/packs fields and is +// never held across I/O; the lifetime of the underlying file descriptors is +// tracked separately by inflight, so Close cannot pull a packfile out from under +// a read that is still running. Once Close has been called, every read fails with +// ErrReaderClosed rather than being misreported as ErrObjectNotFound. type RepositoryReader struct { - loose *LooseReader - packs []*PackReader - mu sync.RWMutex + loose *LooseReader + packs []*PackReader + mu sync.RWMutex + inflight sync.WaitGroup + closed atomic.Bool } // NewRepositoryReader creates an ObjectReader spanning loose objects and packfiles. @@ -103,54 +119,90 @@ func NewRepositoryReader(repo *RepoInfo) (*RepositoryReader, error) { packs: packFiles, } - // Supply the combined reader resolver to packfiles for resolving OBJ_REF_DELTA bases + // Supply the combined reader as the resolver for OBJ_REF_DELTA bases that live + // outside the pack. The field is written here, before the reader is published, + // and is immutable afterwards. for _, p := range reader.packs { - p.SetResolver(reader) + p.resolver = reader } return reader, nil } +// acquire registers an in-flight read and returns a snapshot of the object +// sources. The caller must call r.inflight.Done() once it is finished with them. +// Registering under RLock and releasing the lock before any I/O keeps the +// OBJ_REF_DELTA re-entry deadlock-free: neither Add nor Done ever blocks, and an +// outer read's registration keeps the counter above zero, so a nested Add can +// never race Close's Wait. +func (r *RepositoryReader) acquire() (*LooseReader, []*PackReader, error) { + r.mu.RLock() + defer r.mu.RUnlock() + + if r.closed.Load() { + return nil, nil, ErrReaderClosed + } + r.inflight.Add(1) + return r.loose, r.packs, nil +} + // ReadObject looks up an object by OID across loose objects and packfiles. -// To avoid recursive RLock deadlocks when resolving OBJ_REF_DELTA bases concurrently -// with Close(), the reader list is snapshotted under RLock and unlocked before reading. func (r *RepositoryReader) ReadObject(oid string) (*Object, error) { - r.mu.RLock() - loose := r.loose - packs := r.packs - r.mu.RUnlock() + return r.readObjectDepth(oid, 0) +} - if loose == nil && len(packs) == 0 { - return nil, fmt.Errorf("%w: repository reader is closed", ErrObjectNotFound) +// readObjectDepth resolves oid, carrying depth as the number of delta hops already +// traversed. A packfile resolving an OBJ_REF_DELTA base re-enters here rather than +// through ReadObject, so the maxDeltaDepth ceiling keeps counting across the hop +// and a REF_DELTA cycle terminates with ErrCorruptObject instead of recursing +// until the goroutine stack is exhausted. +func (r *RepositoryReader) readObjectDepth(oid string, depth int) (*Object, error) { + loose, packs, err := r.acquire() + if err != nil { + return nil, fmt.Errorf("%w: %s", err, oid) } + defer r.inflight.Done() + + // A source that claims the object but fails to produce it is a real error, not + // an absent object: remember it and keep looking, but never downgrade it to + // ErrObjectNotFound. + var firstErr error - // Check loose objects first if loose != nil && loose.HasObject(oid) { obj, err := loose.ReadObject(oid) if err == nil { return obj, nil } + firstErr = err } - // Check packfiles for _, pack := range packs { - if pack.HasObject(oid) { - obj, err := pack.ReadObject(oid) - if err == nil { - return obj, nil - } + if !pack.HasObject(oid) { + continue + } + obj, err := pack.readObjectDepth(oid, depth) + if err == nil { + return obj, nil + } + if firstErr == nil { + firstErr = err } } + if firstErr != nil { + return nil, firstErr + } return nil, fmt.Errorf("%w: %s", ErrObjectNotFound, oid) } // HasObject checks if an object exists in loose storage or packfiles. +// A closed reader reports every object as absent. func (r *RepositoryReader) HasObject(oid string) bool { - r.mu.RLock() - loose := r.loose - packs := r.packs - r.mu.RUnlock() + loose, packs, err := r.acquire() + if err != nil { + return false + } + defer r.inflight.Done() if loose != nil && loose.HasObject(oid) { return true @@ -163,23 +215,35 @@ func (r *RepositoryReader) HasObject(oid string) bool { return false } -// Close closes loose and packfile resources. +// Close closes loose and packfile resources. It waits for in-flight reads to +// finish before releasing any file descriptor, so a concurrent read either +// completes against a live packfile or fails with ErrReaderClosed. Close is +// idempotent and safe to call concurrently with reads and with itself. func (r *RepositoryReader) Close() error { r.mu.Lock() - defer r.mu.Unlock() + if r.closed.Load() { + r.mu.Unlock() + return nil + } + r.closed.Store(true) + loose, packs := r.loose, r.packs + r.loose, r.packs = nil, nil + r.mu.Unlock() + + // No new read can register past this point, so this drains the readers that + // are still holding the descriptors below. + r.inflight.Wait() var firstErr error - if r.loose != nil { - if err := r.loose.Close(); err != nil && firstErr == nil { + if loose != nil { + if err := loose.Close(); err != nil { firstErr = err } - r.loose = nil } - for _, pack := range r.packs { + for _, pack := range packs { if err := pack.Close(); err != nil && firstErr == nil { firstErr = err } } - r.packs = nil return firstErr } diff --git a/internal/gitengine/pack.go b/internal/gitengine/pack.go index b6eac43..f6e774f 100644 --- a/internal/gitengine/pack.go +++ b/internal/gitengine/pack.go @@ -9,8 +9,6 @@ import ( "path/filepath" "strings" "sync" - - "github.com/klauspost/compress/zlib" ) var ( @@ -21,13 +19,27 @@ var ( const maxDeltaDepth = 50 -// PackReader provides thread-safe random read access to a Git .pack file using its .idx index. +// deltaBaseResolver resolves an OBJ_REF_DELTA base object that may live outside +// the packfile holding the delta. It carries the current delta-chain depth across +// the hop so that maxDeltaDepth bounds mixed OFS/REF chains and terminates +// REF_DELTA cycles. It is deliberately unexported and narrower than ObjectReader: +// every implementation must be able to continue the depth count, which an +// arbitrary external ObjectReader cannot. +type deltaBaseResolver interface { + readObjectDepth(oid string, depth int) (*Object, error) +} + +// PackReader provides thread-safe random read access to a Git .pack file using its +// .idx index. Every field is immutable once OpenPackfile returns (resolver is wired +// by NewRepositoryReader before the reader is published), so concurrent readers only +// share the pooled zlib readers and the positional reads on file, both of which are +// safe for concurrent use. type PackReader struct { packPath string file *os.File fileSize int64 idx *PackIndex - resolver ObjectReader + resolver deltaBaseResolver zlibPool sync.Pool } @@ -100,11 +112,6 @@ func OpenPackfiles(commonGitDir string) ([]*PackReader, error) { return readers, nil } -// SetResolver sets an ObjectReader to resolve external base objects for OBJ_REF_DELTA. -func (p *PackReader) SetResolver(resolver ObjectReader) { - p.resolver = resolver -} - // HasObject checks if the pack index contains the given OID. func (p *PackReader) HasObject(oid string) bool { return p.idx.HasObject(oid) @@ -112,12 +119,20 @@ func (p *PackReader) HasObject(oid string) bool { // ReadObject reads and decompresses the object corresponding to oid. func (p *PackReader) ReadObject(oid string) (*Object, error) { + return p.readObjectDepth(oid, 0) +} + +// readObjectDepth reads the object corresponding to oid, treating depth as the +// number of delta hops already traversed to get here. Resolving an OBJ_REF_DELTA +// base enters through this method instead of ReadObject so the depth count is not +// reset on every hop. +func (p *PackReader) readObjectDepth(oid string, depth int) (*Object, error) { offset, err := p.idx.FindOffset(oid) if err != nil { return nil, err } - obj, err := p.readObjectAt(offset, 0) + obj, err := p.readObjectAt(offset, depth) if err != nil { return nil, err } @@ -231,7 +246,7 @@ func (p *PackReader) readObjectAt(offset int64, depth int) (*Object, error) { } deltaBuf := GetDeltaBuffer() - targetData, err := ApplyDeltaWithBuffer(*deltaBuf, baseObj.Data, deltaBytes) + targetData, err := ApplyDeltaWithBuffer(deltaBuf, baseObj.Data, deltaBytes) if baseObj.poolBuf != nil { PutDeltaBuffer(baseObj.poolBuf) baseObj.poolBuf = nil @@ -275,16 +290,16 @@ func (p *PackReader) readObjectAt(offset int64, depth int) (*Object, error) { var baseObj *Object if p.resolver != nil { - baseObj, err = p.resolver.ReadObject(baseOID) + baseObj, err = p.resolver.readObjectDepth(baseOID, depth+1) } else { - baseObj, err = p.ReadObject(baseOID) + baseObj, err = p.readObjectDepth(baseOID, depth+1) } if err != nil { return nil, fmt.Errorf("failed to resolve base object %s for ref_delta: %w", baseOID, err) } deltaBuf := GetDeltaBuffer() - targetData, err := ApplyDeltaWithBuffer(*deltaBuf, baseObj.Data, deltaBytes) + targetData, err := ApplyDeltaWithBuffer(deltaBuf, baseObj.Data, deltaBytes) if baseObj.poolBuf != nil { PutDeltaBuffer(baseObj.poolBuf) baseObj.poolBuf = nil @@ -322,23 +337,9 @@ func (p *PackReader) decompressZlib(r io.Reader, size int64) ([]byte, error) { return nil, fmt.Errorf("%w: object size %d exceeds limit", ErrCorruptObject, size) } - var zReader io.ReadCloser - if pooled := p.zlibPool.Get(); pooled != nil { - if zr, ok := pooled.(zlib.Resetter); ok { - if resetErr := zr.Reset(r, nil); resetErr == nil { - if rc, ok := pooled.(io.ReadCloser); ok { - zReader = rc - } - } - } - } - - if zReader == nil { - var zErr error - zReader, zErr = zlib.NewReader(r) - if zErr != nil { - return nil, fmt.Errorf("%w: failed to init zlib decompressor: %v", ErrCorruptObject, zErr) - } + zReader, err := newPooledZlibReader(&p.zlibPool, r) + if err != nil { + return nil, fmt.Errorf("%w: failed to init zlib decompressor: %v", ErrCorruptObject, err) } defer func() { _ = zReader.Close() diff --git a/internal/gitengine/pack_test.go b/internal/gitengine/pack_test.go index 36ec839..a9a189b 100644 --- a/internal/gitengine/pack_test.go +++ b/internal/gitengine/pack_test.go @@ -5,6 +5,7 @@ import ( "crypto/sha1" "encoding/binary" "encoding/hex" + "fmt" "hash/crc32" "os" "path/filepath" @@ -20,6 +21,7 @@ type testPackObj struct { raw []byte // uncompressed payload for regular objects or delta instructions isOfs bool baseOff int64 + baseSHA [20]byte // base object SHA, written for TypeRefDelta objects } func buildTestPackAndIdx(objects []testPackObj) (packData []byte, idxData []byte) { @@ -76,6 +78,10 @@ func buildTestPackAndIdx(objects []testPackObj) (packData []byte, idxData []byte } } + if obj.objType == TypeRefDelta { + packBuf.Write(obj.baseSHA[:]) + } + // Write zlib compressed payload zw := zlib.NewWriter(&packBuf) _, _ = zw.Write(obj.raw) @@ -260,3 +266,91 @@ func TestPackReaderAndOfsDelta(t *testing.T) { t.Errorf("repoReader returned wrong data: %q", objFromRepo.Data) } } + +// writeTestPack writes objs as pack-.{pack,idx} into a fresh temporary git +// object store and returns the common git dir, ready for NewRepositoryReader. +func writeTestPack(t *testing.T, name string, objs []testPackObj) string { + t.Helper() + + packData, idxData := buildTestPackAndIdx(objs) + + gitDir := t.TempDir() + packDir := filepath.Join(gitDir, "objects", "pack") + if err := os.MkdirAll(packDir, 0o755); err != nil { + t.Fatalf("failed to create pack dir: %v", err) + } + if err := os.WriteFile(filepath.Join(packDir, "pack-"+name+".pack"), packData, 0o644); err != nil { + t.Fatalf("failed to write pack: %v", err) + } + if err := os.WriteFile(filepath.Join(packDir, "pack-"+name+".idx"), idxData, 0o644); err != nil { + t.Fatalf("failed to write idx: %v", err) + } + return gitDir +} + +// blobObject returns a pack object entry for content plus its Git blob OID. +func blobObject(content []byte) (testPackObj, [20]byte) { + header := []byte(fmt.Sprintf("blob %d\x00", len(content))) + sha := sha1.Sum(append(header, content...)) + return testPackObj{sha: sha, objType: TypeBlob, raw: content}, sha +} + +func TestPackReaderRefDelta(t *testing.T) { + baseContent := []byte("ref delta base payload") + targetContent := []byte("ref delta base payload!!") + + baseObjEntry, baseSHA := blobObject(baseContent) + + targetHeader := []byte(fmt.Sprintf("blob %d\x00", len(targetContent))) + targetSHA := sha1.Sum(append(targetHeader, targetContent...)) + + var delta []byte + delta = append(delta, encodeLEB128(len(baseContent))...) + delta = append(delta, encodeLEB128(len(targetContent))...) + // Copy all of base: 1 offset byte, 1 size byte + delta = append(delta, 0x80|0x01|0x10, 0, byte(len(baseContent))) + // Insert the two trailing bytes + delta = append(delta, 2, '!', '!') + + objs := []testPackObj{ + baseObjEntry, + {sha: targetSHA, objType: TypeRefDelta, raw: delta, baseSHA: baseSHA}, + } + gitDir := writeTestPack(t, "refdelta", objs) + targetOID := hex.EncodeToString(targetSHA[:]) + + // 1. Through a RepositoryReader, which wires itself in as the pack's resolver. + repoReader, err := NewRepositoryReader(&RepoInfo{CommonGitDir: gitDir}) + if err != nil { + t.Fatalf("NewRepositoryReader failed: %v", err) + } + defer func() { _ = repoReader.Close() }() + + obj, err := repoReader.ReadObject(targetOID) + if err != nil { + t.Fatalf("ReadObject(ref_delta) via RepositoryReader failed: %v", err) + } + if obj.Type != TypeBlob { + t.Errorf("expected TypeBlob, got %v", obj.Type) + } + if !bytes.Equal(obj.Data, targetContent) { + t.Errorf("expected %q, got %q", targetContent, obj.Data) + } + + // 2. Directly through the PackReader, which has no resolver and must fall back + // to looking the base up in its own index. + packDir := filepath.Join(gitDir, "objects", "pack") + pr, err := OpenPackfile(filepath.Join(packDir, "pack-refdelta.pack"), filepath.Join(packDir, "pack-refdelta.idx")) + if err != nil { + t.Fatalf("OpenPackfile failed: %v", err) + } + defer func() { _ = pr.Close() }() + + direct, err := pr.ReadObject(targetOID) + if err != nil { + t.Fatalf("ReadObject(ref_delta) via PackReader failed: %v", err) + } + if !bytes.Equal(direct.Data, targetContent) { + t.Errorf("expected %q, got %q", targetContent, direct.Data) + } +} diff --git a/internal/gitengine/refdelta_cycle_test.go b/internal/gitengine/refdelta_cycle_test.go new file mode 100644 index 0000000..5fbde43 --- /dev/null +++ b/internal/gitengine/refdelta_cycle_test.go @@ -0,0 +1,64 @@ +package gitengine + +import ( + "encoding/hex" + "errors" + "testing" +) + +func mustHexSHA(t *testing.T, s string) [20]byte { + t.Helper() + + raw, err := hex.DecodeString(s) + if err != nil { + t.Fatalf("invalid hex SHA %q: %v", s, err) + } + var sha [20]byte + if len(raw) != len(sha) { + t.Fatalf("expected a 20-byte SHA, got %d bytes", len(raw)) + } + copy(sha[:], raw) + return sha +} + +// TestRefDeltaCycle_ReturnsCorruptObject covers a packfile containing an +// OBJ_REF_DELTA cycle: A is a delta against B and B is a delta against A. +// +// Resolving such a chain used to re-enter the pack through ReadObject, which +// restarted the delta-depth count at zero on every hop, so the maxDeltaDepth +// ceiling never tripped and the recursion continued until the goroutine stack was +// exhausted. That is a fatal runtime error, not a panic: it cannot be recovered +// and it takes the whole process down. The depth must survive the resolver hop so +// the cycle is reported as a corrupt object instead. +func TestRefDeltaCycle_ReturnsCorruptObject(t *testing.T) { + shaA := mustHexSHA(t, "aa11111111111111111111111111111111111111") + shaB := mustHexSHA(t, "bb22222222222222222222222222222222222222") + + // base size 1, target size 1, insert one literal byte + delta := []byte{0x01, 0x01, 0x01, 'x'} + + gitDir := writeTestPack(t, "cycle", []testPackObj{ + {sha: shaA, objType: TypeRefDelta, raw: delta, baseSHA: shaB}, + {sha: shaB, objType: TypeRefDelta, raw: delta, baseSHA: shaA}, + }) + + // NewRepositoryReader wires the combined reader in as each pack's resolver, + // which is the production path the cycle escapes through. + reader, err := NewRepositoryReader(&RepoInfo{CommonGitDir: gitDir}) + if err != nil { + t.Fatalf("NewRepositoryReader failed: %v", err) + } + defer func() { _ = reader.Close() }() + + oidA := hex.EncodeToString(shaA[:]) + obj, err := reader.ReadObject(oidA) + if err == nil { + t.Fatalf("expected an error for the REF_DELTA cycle, got object %+v", obj) + } + if !errors.Is(err, ErrCorruptObject) { + t.Fatalf("expected an error wrapping ErrCorruptObject, got %v", err) + } + if errors.Is(err, ErrObjectNotFound) { + t.Fatalf("a corrupt delta chain must not be reported as a missing object: %v", err) + } +} diff --git a/test/benchmark/search_bench_test.go b/test/benchmark/search_bench_test.go index d042b41..696a986 100644 --- a/test/benchmark/search_bench_test.go +++ b/test/benchmark/search_bench_test.go @@ -207,7 +207,7 @@ func BenchmarkDeltaDecompression(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - target, err := gitengine.ApplyDeltaWithBuffer(buf, base, fullDelta) + target, err := gitengine.ApplyDeltaWithBuffer(&buf, base, fullDelta) if err != nil { b.Fatalf("ApplyDeltaWithBuffer failed: %v", err) }