From 4813c9f1fc7d0bf5cfabbb2a3ee179e35f16847e Mon Sep 17 00:00:00 2001 From: Hammad Majid Date: Sat, 5 Sep 2026 13:06:42 +0500 Subject: [PATCH] fix(gitengine): bound REF_DELTA recursion and harden object-store lifetime A packfile containing an OBJ_REF_DELTA cycle crashed the process with `fatal error: stack overflow`: every REF_DELTA hop re-entered the pack through ObjectReader.ReadObject, which restarted readObjectAt at depth 0, so the maxDeltaDepth ceiling only ever counted intra-pack OFS_DELTA hops. grg reads arbitrary .git directories, so this was attacker-reachable, and a fatal error cannot be recovered by the pipeline worker that hits it. PackReader and RepositoryReader now share an unexported depth-carrying entry point, readObjectDepth, reached through the unexported deltaBaseResolver interface. The delta depth survives the resolver hop, so maxDeltaDepth bounds mixed OFS/REF chains and a cycle terminates as ErrCorruptObject. The exported ObjectReader interface is unchanged. Also in the object store: - RepositoryReader.Close no longer closes packfiles under in-flight readers. Reads register on a WaitGroup under RLock and bail on a closed atomic.Bool; Close flips the flag, drops the lock, drains the readers, and only then closes the files, outside the lock. A shutdown race now reports the new ErrReaderClosed instead of being silently misreported as ErrObjectNotFound, and a source that claims an object but fails to produce it propagates its real error rather than being downgraded to "not found". - SetResolver is gone: an exported unsynchronized mutator on a type documented thread-safe, whose field is read by every pipeline worker. The resolver is now immutable after construction, assigned by NewRepositoryReader before the reader is published. - ApplyDeltaWithBuffer takes *[]byte, so a target that outgrows the pooled 64 KiB buffer writes the grown array back and is actually recycled instead of allocating twice on exactly the objects the pool exists for. - A failed pooled-zlib Reset no longer falls back to zlib.NewReader. Reset has already consumed the RFC-1950 header and installed a bufio read-ahead, so the fallback parsed from a mutated offset and reported a misleading error; the corrupt-object error is returned directly. Both call sites now share newPooledZlibReader, which reaches zlib.NewReader only on an empty pool. - Documented the real concurrency contract on RepositoryReader, LooseReader and PackReader. Tests: buildTestPackAndIdx grows a baseSHA field so it can emit REF_DELTA objects, which previously had no coverage at all. refdelta_cycle_test.go promotes the parked reproduction to a regression test asserting ErrCorruptObject, and TestPackReaderRefDelta covers both the resolver and no-resolver paths. TestConcurrency_RepositoryReader_Deadlock raced a zero-value struct and therefore exercised nothing; it now races 20 readers against 5 concurrent Close calls on a real packfile-backed reader and asserts every read either returns the payload or fails with ErrReaderClosed, never ErrObjectNotFound. Removing the drain from Close makes it fail with "file already closed", so the refcount is load-bearing. Two call sites outside the object store move to the new pointer signature, one line each: internal/gitengine/delta_test.go and test/benchmark/search_bench_test.go. Closes #12 --- internal/gitengine/delta.go | 20 ++-- internal/gitengine/delta_test.go | 2 +- internal/gitengine/hardening_test.go | 90 ++++++++++++++-- internal/gitengine/loose.go | 49 ++++++--- internal/gitengine/object.go | 126 ++++++++++++++++------ internal/gitengine/pack.go | 63 +++++------ internal/gitengine/pack_test.go | 94 ++++++++++++++++ internal/gitengine/refdelta_cycle_test.go | 64 +++++++++++ test/benchmark/search_bench_test.go | 2 +- 9 files changed, 414 insertions(+), 96 deletions(-) create mode 100644 internal/gitengine/refdelta_cycle_test.go 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) }