fix(pack): keep out-of-range offsets out of the header cache (GO-002)
- commit
- 585a2d181f3b4f304bf6b0ceefa6115f4edf1738
- author
- brice · · 2026-10-11 01:07 UTC
- parent
- cb638a6
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +48 -1
M headercache.go +5 -1
| @@ -3,6 +3,7 @@ | |||
| 3 | 3 | package git | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "math" | |
| 6 | 7 | "math/bits" | |
| 7 | 8 | "sync/atomic" | |
| 8 | 9 | ) | |
| @@ -78,6 +79,9 @@ func (s *headerSlot) read(off uint64) (v uint64, ok bool) { | |||
| 78 | 79 | } | |
| 79 | 80 | ||
| 80 | 81 | func (c *headerCache) get(off uint64) (Type, int64, bool) { | |
| 82 | + | if off == math.MaxUint64 { // off+1 would wrap to the empty marker | |
| 83 | + | return 0, 0, false | |
| 84 | + | } | |
| 81 | 85 | i, _ := c.index(off) | |
| 82 | 86 | if v, ok := c.slots[i].read(off); ok { | |
| 83 | 87 | return Type(v & 7), int64(v >> 3), true | |
| @@ -89,7 +93,7 @@ func (c *headerCache) get(off uint64) (Type, int64, bool) { | |||
| 89 | 93 | } | |
| 90 | 94 | ||
| 91 | 95 | func (c *headerCache) put(off uint64, typ Type, size int64) { | |
| 92 | - | if size < 0 || size >= maxCachedSize { | |
| 96 | + | if size < 0 || size >= maxCachedSize || off == math.MaxUint64 { | |
| 93 | 97 | return | |
| 94 | 98 | } | |
| 95 | 99 | i, h := c.index(off) | |
| @@ -3,6 +3,7 @@ | |||||
| 3 | package git | 3 | package git | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "math" | |||
| 6 | "math/bits" | 7 | "math/bits" | ||
| 7 | "sync/atomic" | 8 | "sync/atomic" | ||
| 8 | ) | 9 | ) | ||
| @@ -78,6 +79,9 @@ func (s *headerSlot) read(off uint64) (v uint64, ok bool) { | |||||
| 78 | } | 79 | } | ||
| 79 | 80 | ||||
| 80 | func (c *headerCache) get(off uint64) (Type, int64, bool) { | 81 | func (c *headerCache) get(off uint64) (Type, int64, bool) { | ||
| 82 | + | if off == math.MaxUint64 { // off+1 would wrap to the empty marker | |||
| 83 | + | return 0, 0, false | |||
| 84 | + | } | |||
| 81 | i, _ := c.index(off) | 85 | i, _ := c.index(off) | ||
| 82 | if v, ok := c.slots[i].read(off); ok { | 86 | if v, ok := c.slots[i].read(off); ok { | ||
| 83 | return Type(v & 7), int64(v >> 3), true | 87 | return Type(v & 7), int64(v >> 3), true | ||
| @@ -89,7 +93,7 @@ func (c *headerCache) get(off uint64) (Type, int64, bool) { | |||||
| 89 | } | 93 | } | ||
| 90 | 94 | ||||
| 91 | func (c *headerCache) put(off uint64, typ Type, size int64) { | 95 | func (c *headerCache) put(off uint64, typ Type, size int64) { | ||
| 92 | - | if size < 0 || size >= maxCachedSize { | 96 | + | if size < 0 || size >= maxCachedSize || off == math.MaxUint64 { |
| 93 | return | 97 | return | ||
| 94 | } | 98 | } | ||
| 95 | i, h := c.index(off) | 99 | i, h := c.index(off) | ||
M headercache_test.go +37 -0
| @@ -4,6 +4,7 @@ package git | |||
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | 6 | "fmt" | |
| 7 | + | "math" | |
| 7 | 8 | "sync" | |
| 8 | 9 | "testing" | |
| 9 | 10 | ) | |
| @@ -40,6 +41,15 @@ func TestHeaderCacheGetPut(t *testing.T) { | |||
| 40 | 41 | if typ, size, ok := hc.get(0); !ok || typ != TypeBlob || size != 0 { | |
| 41 | 42 | t.Fatalf("get(0) = %v %v %v", typ, size, ok) | |
| 42 | 43 | } | |
| 44 | + | // An offset that would wrap to the empty marker (off+1 == 0) never hits | |
| 45 | + | // an empty slot and is never stored. | |
| 46 | + | hc.put(math.MaxUint64, TypeBlob, 7) | |
| 47 | + | if _, _, ok := hc.get(math.MaxUint64); ok { | |
| 48 | + | t.Fatal("MaxUint64 cached") | |
| 49 | + | } | |
| 50 | + | if _, _, ok := newHeaderCache(1000).get(math.MaxUint64); ok { | |
| 51 | + | t.Fatal("MaxUint64 hits an empty slot") | |
| 52 | + | } | |
| 43 | 53 | // A size that does not fit a slot is not cached, not truncated. | |
| 44 | 54 | hc.put(99, TypeBlob, maxCachedSize) | |
| 45 | 55 | if _, _, ok := hc.get(99); ok { | |
| @@ -139,3 +149,30 @@ func TestObjectHeaderCached(t *testing.T) { | |||
| 139 | 149 | t.Fatal("no deltified object in the fixture") | |
| 140 | 150 | } | |
| 141 | 151 | } | |
| 152 | + | ||
| 153 | + | // An offset past the end of the pack is an error, never an answer from an | |
| 154 | + | // empty cache slot (math.MaxUint64 + 1 wraps to the empty marker). | |
| 155 | + | func TestPackHeaderOutOfRange(t *testing.T) { | |
| 156 | + | f := newFx(t) | |
| 157 | + | for i := 0; i < 5; i++ { | |
| 158 | + | f.write("f.txt", append(textLines(300, 0), fmt.Sprintf("rev %d\n", i)...), 0o644) | |
| 159 | + | f.commit(fmt.Sprintf("commit %d", i)) | |
| 160 | + | } | |
| 161 | + | f.git("repack", "-adf", "--depth=50", "--window=250") | |
| 162 | + | r := f.open() | |
| 163 | + | p := r.snap.Load().packs[0] | |
| 164 | + | for i := range p.idx.n { // allocate the cache | |
| 165 | + | off, _ := p.idx.offset(i) | |
| 166 | + | if _, _, err := p.header(off); err != nil { | |
| 167 | + | t.Fatal(err) | |
| 168 | + | } | |
| 169 | + | } | |
| 170 | + | if p.hdrs.Load() == nil { | |
| 171 | + | t.Skip("no deltified object, no cache") | |
| 172 | + | } | |
| 173 | + | for _, off := range []uint64{uint64(len(p.data)), math.MaxUint64 - 1, math.MaxUint64} { | |
| 174 | + | if typ, size, err := p.header(off); err == nil { | |
| 175 | + | t.Errorf("header(%d) = %v %d, want an error", off, typ, size) | |
| 176 | + | } | |
| 177 | + | } | |
| 178 | + | } | |
| @@ -4,6 +4,7 @@ package git | |||||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | "fmt" | 6 | "fmt" | ||
| 7 | + | "math" | |||
| 7 | "sync" | 8 | "sync" | ||
| 8 | "testing" | 9 | "testing" | ||
| 9 | ) | 10 | ) | ||
| @@ -40,6 +41,15 @@ func TestHeaderCacheGetPut(t *testing.T) { | |||||
| 40 | if typ, size, ok := hc.get(0); !ok || typ != TypeBlob || size != 0 { | 41 | if typ, size, ok := hc.get(0); !ok || typ != TypeBlob || size != 0 { | ||
| 41 | t.Fatalf("get(0) = %v %v %v", typ, size, ok) | 42 | t.Fatalf("get(0) = %v %v %v", typ, size, ok) | ||
| 42 | } | 43 | } | ||
| 44 | + | // An offset that would wrap to the empty marker (off+1 == 0) never hits | |||
| 45 | + | // an empty slot and is never stored. | |||
| 46 | + | hc.put(math.MaxUint64, TypeBlob, 7) | |||
| 47 | + | if _, _, ok := hc.get(math.MaxUint64); ok { | |||
| 48 | + | t.Fatal("MaxUint64 cached") | |||
| 49 | + | } | |||
| 50 | + | if _, _, ok := newHeaderCache(1000).get(math.MaxUint64); ok { | |||
| 51 | + | t.Fatal("MaxUint64 hits an empty slot") | |||
| 52 | + | } | |||
| 43 | // A size that does not fit a slot is not cached, not truncated. | 53 | // A size that does not fit a slot is not cached, not truncated. | ||
| 44 | hc.put(99, TypeBlob, maxCachedSize) | 54 | hc.put(99, TypeBlob, maxCachedSize) | ||
| 45 | if _, _, ok := hc.get(99); ok { | 55 | if _, _, ok := hc.get(99); ok { | ||
| @@ -139,3 +149,30 @@ func TestObjectHeaderCached(t *testing.T) { | |||||
| 139 | t.Fatal("no deltified object in the fixture") | 149 | t.Fatal("no deltified object in the fixture") | ||
| 140 | } | 150 | } | ||
| 141 | } | 151 | } | ||
| 152 | + | ||||
| 153 | + | // An offset past the end of the pack is an error, never an answer from an | |||
| 154 | + | // empty cache slot (math.MaxUint64 + 1 wraps to the empty marker). | |||
| 155 | + | func TestPackHeaderOutOfRange(t *testing.T) { | |||
| 156 | + | f := newFx(t) | |||
| 157 | + | for i := 0; i < 5; i++ { | |||
| 158 | + | f.write("f.txt", append(textLines(300, 0), fmt.Sprintf("rev %d\n", i)...), 0o644) | |||
| 159 | + | f.commit(fmt.Sprintf("commit %d", i)) | |||
| 160 | + | } | |||
| 161 | + | f.git("repack", "-adf", "--depth=50", "--window=250") | |||
| 162 | + | r := f.open() | |||
| 163 | + | p := r.snap.Load().packs[0] | |||
| 164 | + | for i := range p.idx.n { // allocate the cache | |||
| 165 | + | off, _ := p.idx.offset(i) | |||
| 166 | + | if _, _, err := p.header(off); err != nil { | |||
| 167 | + | t.Fatal(err) | |||
| 168 | + | } | |||
| 169 | + | } | |||
| 170 | + | if p.hdrs.Load() == nil { | |||
| 171 | + | t.Skip("no deltified object, no cache") | |||
| 172 | + | } | |||
| 173 | + | for _, off := range []uint64{uint64(len(p.data)), math.MaxUint64 - 1, math.MaxUint64} { | |||
| 174 | + | if typ, size, err := p.header(off); err == nil { | |||
| 175 | + | t.Errorf("header(%d) = %v %d, want an error", off, typ, size) | |||
| 176 | + | } | |||
| 177 | + | } | |||
| 178 | + | } | |||
M pack.go +6 -0
| @@ -258,6 +258,12 @@ func (p *pack) baseOffset(h *entryHdr, self uint64) (uint64, error) { | |||
| 258 | 258 | // header returns the type and size of the object at off. The result of a | |
| 259 | 259 | // deltified object, which costs an inflate to find, is cached per pack. | |
| 260 | 260 | func (p *pack) header(off uint64) (Type, int64, error) { | |
| 261 | + | if off >= uint64(len(p.data)) { | |
| 262 | + | // Not an entry of this pack: let readHeader reject it, and keep the | |
| 263 | + | // cache to real offsets. | |
| 264 | + | typ, size, _, err := p.readHeader(off) | |
| 265 | + | return typ, size, err | |
| 266 | + | } | |
| 261 | 267 | hc := p.hdrs.Load() | |
| 262 | 268 | if hc != nil { | |
| 263 | 269 | if typ, size, ok := hc.get(off); ok { | |
| @@ -258,6 +258,12 @@ func (p *pack) baseOffset(h *entryHdr, self uint64) (uint64, error) { | |||||
| 258 | // header returns the type and size of the object at off. The result of a | 258 | // header returns the type and size of the object at off. The result of a | ||
| 259 | // deltified object, which costs an inflate to find, is cached per pack. | 259 | // deltified object, which costs an inflate to find, is cached per pack. | ||
| 260 | func (p *pack) header(off uint64) (Type, int64, error) { | 260 | func (p *pack) header(off uint64) (Type, int64, error) { | ||
| 261 | + | if off >= uint64(len(p.data)) { | |||
| 262 | + | // Not an entry of this pack: let readHeader reject it, and keep the | |||
| 263 | + | // cache to real offsets. | |||
| 264 | + | typ, size, _, err := p.readHeader(off) | |||
| 265 | + | return typ, size, err | |||
| 266 | + | } | |||
| 261 | hc := p.hdrs.Load() | 267 | hc := p.hdrs.Load() | ||
| 262 | if hc != nil { | 268 | if hc != nil { | ||
| 263 | if typ, size, ok := hc.get(off); ok { | 269 | if typ, size, ok := hc.get(off); ok { | ||