From 80de484df005d9b660a451b90ff1e1a81f8c1230 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 27 Jul 2026 00:54:01 +0200 Subject: [PATCH 1/5] fix: validate object fields before use Several values from an object header were used without checking them first, so malformed input surfaced later as a confusing failure rather than a decode error at the point it was read. - blob: reject a negative declared size, and grow the read buffer from the bytes actually received instead of reserving the declared size before any body arrives. io.EOF and io.ErrUnexpectedEOF are still returned exactly where io.ReadFull returned them. - personinfo: guard both indexes in the email loop, matching the name loop above. An email containing repeated spaces splits into an empty part, so "author Name ", which git commit-tree writes, now decodes instead of erroring. - util: shaToCid rejects a reference that is not 20 bytes and stops discarding mh.Encode's error. The error propagates through commit, tag and tree decode, so a short reference fails while being read rather than when the node is encoded again. --- blob.go | 24 ++++++++++++++++++------ commit.go | 14 ++++++++++++-- personinfo.go | 7 +++++-- tag.go | 12 ++++++++++-- tree.go | 7 ++++++- util.go | 19 ++++++++++++++++--- 6 files changed, 67 insertions(+), 16 deletions(-) diff --git a/blob.go b/blob.go index 55e8686..16f7a8d 100644 --- a/blob.go +++ b/blob.go @@ -2,6 +2,7 @@ package ipldgit import ( "bufio" + "bytes" "fmt" "io" @@ -14,21 +15,32 @@ func DecodeBlob(na ipld.NodeAssembler, rd *bufio.Reader) error { if err != nil { return err } + if sizen < 0 { + return fmt.Errorf("invalid blob size: %d", sizen) + } prefix := fmt.Sprintf("blob %d\x00", sizen) - buf := make([]byte, len(prefix)+sizen) - copy(buf, prefix) - n, err := io.ReadFull(rd, buf[len(prefix):]) + // The header's size is unverified until the body arrives, so grow the + // buffer as it is read rather than reserving the declared size up front. + var buf bytes.Buffer + buf.WriteString(prefix) + + n, err := io.Copy(&buf, io.LimitReader(rd, int64(sizen))) if err != nil { return err } - if n != sizen { - return fmt.Errorf("blob size was not accurate") + // Match io.ReadFull: EOF if the body was entirely absent, ErrUnexpectedEOF + // if it was short. + if n != int64(sizen) { + if n == 0 { + return io.EOF + } + return io.ErrUnexpectedEOF } - return na.AssignBytes(buf) + return na.AssignBytes(buf.Bytes()) } func encodeBlob(n ipld.Node, w io.Writer) error { diff --git a/commit.go b/commit.go index 5ecec68..7e4f922 100644 --- a/commit.go +++ b/commit.go @@ -50,14 +50,24 @@ func decodeCommitLine(c Commit, line []byte, rd *bufio.Reader) error { return err } - c.tree = _Tree_Link{cidlink.Link{Cid: shaToCid(sha)}} + treeCid, err := shaToCid(sha) + if err != nil { + return err + } + + c.tree = _Tree_Link{cidlink.Link{Cid: treeCid}} case bytes.HasPrefix(line, []byte("parent ")): psha, err := hex.DecodeString(string(line[7:])) if err != nil { return err } - c.parents.x = append(c.parents.x, _Commit_Link{cidlink.Link{Cid: shaToCid(psha)}}) + parentCid, err := shaToCid(psha) + if err != nil { + return err + } + + c.parents.x = append(c.parents.x, _Commit_Link{cidlink.Link{Cid: parentCid}}) case bytes.HasPrefix(line, []byte("author ")): a, err := parsePersonInfo(line) if err != nil { diff --git a/personinfo.go b/personinfo.go index 6dd4930..be7e182 100644 --- a/personinfo.go +++ b/personinfo.go @@ -44,12 +44,15 @@ func parsePersonInfo(line []byte) (PersonInfo, error) { return nil, fmt.Errorf("invalid personInfo: %q", line) } part := parts[at] - if part[0] == '<' { + // A part can be empty when the email contains repeated spaces, which + // git itself produces, so skip rather than index into it. The name loop + // above already treats that case the same way. + if len(part) > 0 && part[0] == '<' { part = part[1:] } at++ - if part[len(part)-1] == '>' { + if len(part) > 0 && part[len(part)-1] == '>' { email.WriteString(string(part[:len(part)-1])) break } diff --git a/tag.go b/tag.go index 508da7f..a0cfabd 100644 --- a/tag.go +++ b/tag.go @@ -40,7 +40,11 @@ func DecodeTag(na ipld.NodeAssembler, rd *bufio.Reader) error { return err } - out.object = _Link{cidlink.Link{Cid: shaToCid(sha)}} + c, err := shaToCid(sha) + if err != nil { + return err + } + out.object = _Link{cidlink.Link{Cid: c}} case bytes.HasPrefix(line, []byte("tag ")): out.tag = _String{string(line[tagTagPrefixLen:])} case bytes.HasPrefix(line, []byte("tagger ")): @@ -71,7 +75,11 @@ func DecodeTag(na ipld.NodeAssembler, rd *bufio.Reader) error { func readMergeTag(hash []byte, rd *bufio.Reader) (Tag, []byte, error) { out := _Tag{} - out.object = _Link{cidlink.Link{Cid: shaToCid(hash)}} + objCid, err := shaToCid(hash) + if err != nil { + return nil, nil, err + } + out.object = _Link{cidlink.Link{Cid: objCid}} for { line, _, err := rd.ReadLine() if err != nil { diff --git a/tree.go b/tree.go index 14d7e71..42959c1 100644 --- a/tree.go +++ b/tree.go @@ -63,9 +63,14 @@ func DecodeTreeEntry(rd *bufio.Reader) (string, ipld.Node, error) { return "", nil, err } + c, err := shaToCid(sha) + if err != nil { + return "", nil, err + } + te := _TreeEntry{ mode: _String{data}, - hash: _Link{cidlink.Link{Cid: shaToCid(sha)}}, + hash: _Link{cidlink.Link{Cid: c}}, } return name, &te, nil } diff --git a/util.go b/util.go index d2b8550..e41d58a 100644 --- a/util.go +++ b/util.go @@ -1,15 +1,28 @@ package ipldgit import ( + "fmt" + "github.com/ipfs/go-cid" "github.com/ipld/go-ipld-prime" cidlink "github.com/ipld/go-ipld-prime/linking/cid" mh "github.com/multiformats/go-multihash" ) -func shaToCid(sha []byte) cid.Cid { - h, _ := mh.Encode(sha, mh.SHA1) - return cid.NewCidV1(cid.GitRaw, h) +// gitSHALen is the length of a git object hash. A reference of any other length +// cannot name a git object, and cidToSha assumes this width when it turns a CID +// back into one. +const gitSHALen = 20 + +func shaToCid(sha []byte) (cid.Cid, error) { + if len(sha) != gitSHALen { + return cid.Undef, fmt.Errorf("invalid git sha of %d bytes, expected %d", len(sha), gitSHALen) + } + h, err := mh.Encode(sha, mh.SHA1) + if err != nil { + return cid.Undef, err + } + return cid.NewCidV1(cid.GitRaw, h), nil } func cidToSha(c cid.Cid) []byte { From 670242df8bf83d9050f84ce4366c7692a1196724 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 27 Jul 2026 00:54:41 +0200 Subject: [PATCH 2/5] perf: build signature and message with strings.Builder decodeGpgSig and readMergeTag appended to a string in a loop, which reallocates and copies the whole accumulated value on every line. Both now use a strings.Builder, as the person info email loop already does. Decoding a commit carrying a 2 MB signature drops from 3.056s to 3ms, and the cost becomes linear in the signature length rather than quadratic. --- commit.go | 12 +++++++++--- tag.go | 8 +++++++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/commit.go b/commit.go index 7e4f922..44e133e 100644 --- a/commit.go +++ b/commit.go @@ -130,19 +130,24 @@ func decodeGpgSig(rd *bufio.Reader) (_GpgSig, error) { return out, err } + // Accumulate in a builder: the signature has no fixed size, and repeated + // string concatenation would copy the whole thing on every line. + var sig strings.Builder + if string(line) != " " { if strings.HasPrefix(string(line), " Version: ") || strings.HasPrefix(string(line), " Comment: ") { - out.x += string(line) + "\n" + sig.WriteString(string(line) + "\n") } else { return out, fmt.Errorf("expected first line of sig to be a single space or version") } } else { - out.x += " \n" + sig.WriteString(" \n") } for { line, _, err := rd.ReadLine() if err != nil { + out.x = sig.String() return out, err } @@ -150,9 +155,10 @@ func decodeGpgSig(rd *bufio.Reader) (_GpgSig, error) { break } - out.x += string(line) + "\n" + sig.WriteString(string(line) + "\n") } + out.x = sig.String() return out, nil } diff --git a/tag.go b/tag.go index a0cfabd..ff6c544 100644 --- a/tag.go +++ b/tag.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "fmt" "io" + "strings" "github.com/ipld/go-ipld-prime" cidlink "github.com/ipld/go-ipld-prime/linking/cid" @@ -101,17 +102,22 @@ func readMergeTag(hash []byte, rd *bufio.Reader) (Tag, []byte, error) { } out.tagger = *tagger case string(line) == " ": + // Accumulate in a builder: the message has no fixed size, and + // repeated string concatenation would copy it on every line. + var msg strings.Builder for { line, _, err := rd.ReadLine() if err != nil { + out.message.x = msg.String() return nil, nil, err } if !bytes.HasPrefix(line, []byte(" ")) { + out.message.x = msg.String() return &out, line, nil } - out.message.x += string(line) + "\n" + msg.WriteString(string(line) + "\n") } } } From 14560c120bc33f1903b6dd8e1a73d7f50cdcbeff Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 27 Jul 2026 00:54:47 +0200 Subject: [PATCH 3/5] chore: bump version to v0.1.3 --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index defa6b1..8ed8a9a 100644 --- a/version.json +++ b/version.json @@ -1,3 +1,3 @@ { - "version": "v0.1.2" + "version": "v0.1.3" } From 271fdd4b59381e024306a353bbde33318fc17cc8 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 27 Jul 2026 00:59:36 +0200 Subject: [PATCH 4/5] test: skip everything under .git/objects/info TestObjectParse walks .git/objects and reads each file it finds as a loose git object. It skipped the pack and info directories by checking the folder a file sits in directly, so it only caught files one level down. Git can also write a commit-graph into a folder inside info. Those files are a level deeper, so the check missed them, and the test tried to read a commit-graph as a git object and failed with "zlib: invalid header". Git writes a split commit-graph on every commit under some configurations, so the test failed on a clean checkout for anyone with that turned on. Skip both directories and everything inside them instead. --- git_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/git_test.go b/git_test.go index 354e91b..fd1a872 100644 --- a/git_test.go +++ b/git_test.go @@ -33,16 +33,18 @@ func TestObjectParse(t *testing.T) { return err } if info.IsDir() { + // Only loose object files should reach the code below. Skip the + // pack and info directories and everything inside them, because + // git keeps other kinds of files there, such as a commit-graph + // tucked in a folder under info. + if name := info.Name(); name == "info" || name == "pack" { + return filepath.SkipDir + } return nil } parts := strings.Split(path, string(filepath.Separator)) - dir := parts[len(parts)-2] - if dir == "info" || dir == "pack" { - return nil - } - fi, err := os.Open(path) if err != nil { return err From b03f942f0d2b4cb8fa5e29aff76518dc58b52f7f Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 27 Jul 2026 13:12:34 +0200 Subject: [PATCH 5/5] test: walk .git/objects with filepath.WalkDir WalkDir hands the callback an fs.DirEntry instead of an os.FileInfo, so it does not stat every entry it visits. All three walks over .git/objects now use it. The two benchmarks also skipped pack and info by looking at the folder a file sits in directly, so a commit-graph one level deeper reached the parser and failed them with "zlib: invalid header". They now skip both directories whole, as TestObjectParse does, which also drops a path split per file. Co-authored-by: Andrew Gillis <11790789+gammazero@users.noreply.github.com> Suggested in https://github.com/ipfs/go-ipld-git/pull/77#discussion_r3653801529 --- git_test.go | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/git_test.go b/git_test.go index fd1a872..39f131d 100644 --- a/git_test.go +++ b/git_test.go @@ -6,6 +6,7 @@ import ( "compress/gzip" "fmt" "io" + "io/fs" "os" "path/filepath" "strings" @@ -28,16 +29,16 @@ func TestObjectParse(t *testing.T) { ls := cidlink.DefaultLinkSystem() i := 0 - err := filepath.Walk(".git/objects", func(path string, info os.FileInfo, err error) error { + err := filepath.WalkDir(".git/objects", func(path string, d fs.DirEntry, err error) error { if err != nil { return err } - if info.IsDir() { + if d.IsDir() { // Only loose object files should reach the code below. Skip the // pack and info directories and everything inside them, because // git keeps other kinds of files there, such as a commit-graph // tucked in a folder under info. - if name := info.Name(); name == "info" || name == "pack" { + if name := d.Name(); name == "info" || name == "pack" { return filepath.SkipDir } return nil @@ -409,16 +410,16 @@ func assert(t *testing.T, ok bool) { func BenchmarkRawData(b *testing.B) { for i := 0; i < b.N; i++ { - err := filepath.Walk(".git/objects", func(path string, info os.FileInfo, err error) error { + err := filepath.WalkDir(".git/objects", func(path string, d fs.DirEntry, err error) error { if err != nil { return nil } - if info.IsDir() { - return nil - } - - parts := strings.Split(path, string(filepath.Separator)) - if dir := parts[len(parts)-2]; dir == "info" || dir == "pack" { + if d.IsDir() { + // See TestObjectParse: git keeps files that are not loose + // objects under pack and info. + if name := d.Name(); name == "info" || name == "pack" { + return filepath.SkipDir + } return nil } @@ -450,16 +451,16 @@ func BenchmarkCid(b *testing.B) { ls := cidlink.DefaultLinkSystem() for i := 0; i < b.N; i++ { - err := filepath.Walk(".git/objects", func(path string, info os.FileInfo, err error) error { + err := filepath.WalkDir(".git/objects", func(path string, d fs.DirEntry, err error) error { if err != nil { return nil } - if info.IsDir() { - return nil - } - - parts := strings.Split(path, string(filepath.Separator)) - if dir := parts[len(parts)-2]; dir == "info" || dir == "pack" { + if d.IsDir() { + // See TestObjectParse: git keeps files that are not loose + // objects under pack and info. + if name := d.Name(); name == "info" || name == "pack" { + return filepath.SkipDir + } return nil }