Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions internal/gitengine/delta.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion internal/gitengine/delta_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
90 changes: 81 additions & 9 deletions internal/gitengine/hardening_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package gitengine
import (
"bytes"
"encoding/binary"
"encoding/hex"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -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
Expand Down
49 changes: 32 additions & 17 deletions internal/gitengine/loose.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
Loading
Loading