From 8c06407915b4d45c89048b0d321ab8f1ee299ced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Mur=C3=A9?= Date: Sat, 10 Oct 2026 18:31:13 +0200 Subject: [PATCH] lower load limits to 64k entries and 4 MiB --- docs/COMPATIBILITY.md | 4 ++-- docs/GIT-BEHAVIOUR.md | 2 +- docs/REQUIREMENTS.md | 2 +- env.go | 2 +- env_test.go | 16 ++++++++-------- errors.go | 4 ++-- file.go | 4 ++-- file_test.go | 21 ++++++++++++--------- load.go | 12 ++++++++---- 9 files changed, 37 insertions(+), 30 deletions(-) diff --git a/docs/COMPATIBILITY.md b/docs/COMPATIBILITY.md index 5461d34..b8e49fb 100644 --- a/docs/COMPATIBILITY.md +++ b/docs/COMPATIBILITY.md @@ -52,7 +52,7 @@ in [GIT-BEHAVIOUR.md](GIT-BEHAVIOUR.md). | Case | git | This library | Why | |---|---|---|---| | Include depth | at most 10 | the same | 1 | -| Include fan-out, FIFOs, devices | unbounded work, hangs on a FIFO | at most 1000 files, 16 MiB and 2²⁰ entries per load; only regular files | X3 | +| Include fan-out, FIFOs, devices | unbounded work, hangs on a FIFO | at most 1000 files, 4 MiB and 2¹⁶ entries per load; only regular files | X3 | | Missing include | ignored | the same | 1 | | Global file not readable (permission) | skipped, with a warning | skipped; also the system file in `LoadProtected` | 1 | | `.` and `..` in a `gitdir:` pattern | compared as written, never normalised | the same | 1 | @@ -107,7 +107,7 @@ in [GIT-BEHAVIOUR.md](GIT-BEHAVIOUR.md). | More than 5 levels of symbolic links | replaces the 5th link with a regular file | error | git destroys a link it meant to follow | | Permissions | kept exactly | the same | 1 | | Content unchanged by the edit | file rewritten | not written | E7 | -| File larger than 16 MiB, or with more than 2²⁰ entries | edited | `ErrLoadLimit` | X3 — *safe default* | +| File larger than 4 MiB, or with more than 2¹⁶ entries | edited | `ErrLoadLimit` | X3 — *safe default* | | Windows: another process holding the file for a moment | — | open and rename retried for up to a second | concurrent writers would otherwise fail | ## Adding a case diff --git a/docs/GIT-BEHAVIOUR.md b/docs/GIT-BEHAVIOUR.md index f4c31a8..97025fc 100644 --- a/docs/GIT-BEHAVIOUR.md +++ b/docs/GIT-BEHAVIOUR.md @@ -221,7 +221,7 @@ the same results. | `path` is a directory | `warning: unable to access 'd': Is a directory`, then `fatal: bad config line 2` *(2.47.3)* | | `path` is a FIFO | hangs until something writes to the FIFO *(2.47.3)* — this library refuses it (X3) | | `path = /dev/zero` | `fatal: bad config line 1 in file /dev/zero` (NUL is a syntax error) *(2.47.3)* | -| 7 files, each including the next 10 times, the last holding `a.k` | no limit other than depth: 10ⁿ values for n levels — n = 3: 1,000 values in 0.01 s; n = 6: 1,000,000 in 7 s; n = 10 would be ~10¹⁰ values, hours of work and memory exhaustion from 3 KB of files *(2.47.3)* — this library stops at 1000 files or 64 MiB read per load (X3) | +| 7 files, each including the next 10 times, the last holding `a.k` | no limit other than depth: 10ⁿ values for n levels — n = 3: 1,000 values in 0.01 s; n = 6: 1,000,000 in 7 s; n = 10 would be ~10¹⁰ values, hours of work and memory exhaustion from 3 KB of files *(2.47.3)* — this library stops at 1000 files, 4 MiB or 2¹⁶ entries read per load (X3) | ### `includeIf` (repository at `$S/work/proj`, branch `main`) diff --git a/docs/REQUIREMENTS.md b/docs/REQUIREMENTS.md index 516771e..af5f9d9 100644 --- a/docs/REQUIREMENTS.md +++ b/docs/REQUIREMENTS.md @@ -90,7 +90,7 @@ and callers must be able to tell whether to trust it. |---|---|---| | X1 | No injection through edits: whatever key, subsection and value an edit is given, the edited file parses to the previous entries with exactly that change. A subsection containing a newline or NUL is rejected (as by git). Sections to remove or rename are found by parsing, where git scans lines: git takes a line starting with `[` inside a continued value for a header (removing the entries after it, or rewriting the value), and misses a header after a BOM or after another header on the same line. A file holding only a BOM keeps it first, where git writes the new section before it and the file no longer parses. *verified* Fuzzed: parse ∘ edit ∘ parse, for key and section edits. | MUST | | X2 | Parsing untrusted input never panics and is linear in time and memory. Parser and edits are fuzzed continuously. | MUST | -| X3 | Bounded loads: git's include depth limit, plus at most 1000 files, 16 MiB and 2²⁰ entries and distinct section headers read by one load, counting every read (an include fan-out of 10 per level makes git read 10ⁿ files from a few KB, *verified*; an entry costs about a hundred times its size in memory, so the entry count, not the bytes, bounds memory); `Parse` and edits apply the same limits to their one file; configuration files and includes must be regular files (git hangs on a FIFO and fails on a directory, *verified*). A deliberate improvement over git, reached only by files built to reach it. | MUST | +| X3 | Bounded loads: git's include depth limit, plus at most 1000 files, 4 MiB and 2¹⁶ entries and distinct section headers read by one load, counting every read (an include fan-out of 10 per level makes git read 10ⁿ files from a few KB, *verified*; an entry costs about a hundred times its size in memory, so the entry count, not the bytes, bounds memory); `Parse` and edits apply the same limits to their one file; configuration files and includes must be regular files (git hangs on a FIFO and fails on a directory, *verified*). A deliberate improvement over git, reached only by files built to reach it. | MUST | | X4 | Errors do not leak or spoof: a syntax error never quotes the file's content (an include may point at a secret file), values in messages are quoted with `%q`, and paths, which a configuration file can choose, have their control characters (newline and tab included), bidirectional controls and non-UTF-8 bytes replaced with `?`: stricter than git 2.47.3 and 2.56.0, which replace only C0 controls but tab and newline, and DEL (*verified*); no terminal escape sequences, forged lines or disguised paths. | MUST | | X5 | Trust is checked before the repository is read, as git does: `Discover` refuses (`ErrUntrusted`) a repository that is neither owned by the current user (on Unix, also the `SUDO_UID` user for root; on Windows, the Administrators group for an administrator) nor listed in `safe.directory`, with git's rules for which paths are checked and listed. `safe.directory` is read from the protected scopes only (`Env.LoadProtected`: system, global, command; never `Parse` content or an explicit file), never from the repository's configuration. An `Env` filled by hand is not checked, as git does not check `GIT_DIR`. Each entry's scope is known. *verified* | MUST | | X6 | Writes cannot be redirected through the lock file: it is created with `O_CREATE\|O_EXCL` and never through a symbolic link. Following a link at the config path itself is git's behaviour (W7) and is kept; the documentation says that only X5 protects against a hostile `.git` directory. | MUST | diff --git a/env.go b/env.go index 60a0cad..5521ecb 100644 --- a/env.go +++ b/env.go @@ -147,7 +147,7 @@ func Discover(dir string) (Env, error) { // true), up to 10 levels deep (ErrIncludeDepth). Two safety bounds git lacks // are added: an include must be a regular file (ErrNotRegular, so a FIFO or a // device cannot hang or exhaust the process), and one load reads at most -// 1000 files, 16 MiB and 2²⁰ entries and distinct section headers in total +// 1000 files, 4 MiB and 2¹⁶ entries and distinct section headers in total // from files (ErrLoadLimit, so that a file including itself many times cannot // multiply the work exponentially, and a file of tiny entries cannot exhaust // memory). diff --git a/env_test.go b/env_test.go index 9794740..40ef0fe 100644 --- a/env_test.go +++ b/env_test.go @@ -469,20 +469,20 @@ func TestEnvLoadLimits(t *testing.T) { } t.Run("entries", func(t *testing.T) { - limitErr(t, map[string]string{"main": "[a]\n" + strings.Repeat("k\n", 1<<20+1)}) + limitErr(t, map[string]string{"main": "[a]\n" + strings.Repeat("k\n", 1<<16+1)}) }) t.Run("entries across includes", func(t *testing.T) { - // 600 reads of 2000 entries, under the file and byte limits. + // 40 reads of 2000 entries, under the file and byte limits. limitErr(t, map[string]string{ - "main": strings.Repeat("[include]\n\tpath = inc\n", 600), + "main": strings.Repeat("[include]\n\tpath = inc\n", 40), "inc": "[a]\n" + strings.Repeat("k\n", 2000), }) }) t.Run("distinct headers", func(t *testing.T) { var sb strings.Builder - for i := range 1<<20 + 1 { + for i := range 1<<16 + 1 { fmt.Fprintf(&sb, "[a \"%x\"]", i) } limitErr(t, map[string]string{"main": sb.String()}) @@ -490,7 +490,7 @@ func TestEnvLoadLimits(t *testing.T) { t.Run("repeated headers are free", func(t *testing.T) { dir := tempDir(t) - writeFile(t, filepath.Join(dir, "main"), strings.Repeat("[a]", 1<<20+1)+"k\n") + writeFile(t, filepath.Join(dir, "main"), strings.Repeat("[a]", 1<<16+1)+"k\n") c, err := gitconfig.Env{GlobalConfig: []string{filepath.Join(dir, "main")}}.Load() require.NoError(t, err) assert.Len(t, c.GetAll("a.k"), 1) @@ -522,7 +522,7 @@ func TestEnvLoadLimits(t *testing.T) { // One costly pattern, then many paths under it: the condition is // evaluated once, not once per path. dir := tempDir(t) - writeFile(t, filepath.Join(dir, "main"), "[includeIf \"gitdir:"+strings.Repeat("a*", 1<<17)+"\"]\n"+strings.Repeat("path =\n", 1<<16)) + writeFile(t, filepath.Join(dir, "main"), "[includeIf \"gitdir:"+strings.Repeat("a*", 1<<17)+"\"]\n"+strings.Repeat("path =\n", 1<<15)) initRepo(t, filepath.Join(dir, ".git"), "") _, err := gitconfig.Env{GitDir: filepath.Join(dir, ".git"), GlobalConfig: []string{filepath.Join(dir, "main")}}.Load() @@ -536,9 +536,9 @@ func TestEnvLoadLimits(t *testing.T) { // place in the index of the Config. func TestLoadMemory(t *testing.T) { if testing.Short() { - t.Skip("allocates a few hundred MB") + t.Skip("allocates about 50 MB") } - const n = 1 << 17 + const n = 1 << 15 var sb strings.Builder sb.WriteString("[a]\n") for i := range n { diff --git a/errors.go b/errors.go index 4965ba2..6e33d77 100644 --- a/errors.go +++ b/errors.go @@ -58,10 +58,10 @@ var ( ErrIncludeDepth = errors.New("gitconfig: exceeded maximum include depth") // ErrLoadLimit is returned when one load would read more than 1000 - // files, 16 MiB, or 2²⁰ entries and distinct section headers in total + // files, 4 MiB, or 2¹⁶ entries and distinct section headers in total // from files (entries of Env.Command are not counted). // Parse applies the byte and entry limits to its one file, and keeps at - // most 2²² pieces of it (blank lines, comments, ...); an edit that would + // most 2¹⁸ pieces of it (blank lines, comments, ...); an edit that would // take a File beyond these limits fails with it. ErrLoadLimit = errors.New("gitconfig: exceeded load limits") diff --git a/file.go b/file.go index ac443e6..affd73a 100644 --- a/file.go +++ b/file.go @@ -57,8 +57,8 @@ type File struct { // // data may come from anyone (a .gitmodules blob from a remote): Parse never // panics, and its time and memory are linear in len(data). It applies the -// limits of a load to its one file: more than 16 MiB, more than 2²⁰ entries -// and distinct section headers, or more than 2²² pieces (blank lines, +// limits of a load to its one file: more than 4 MiB, more than 2¹⁶ entries +// and distinct section headers, or more than 2¹⁸ pieces (blank lines, // comments, ...) is a *FileError wrapping ErrLoadLimit. func Parse(name string, data []byte) (*File, error) { if len(data) > maxLoadBytes { diff --git a/file_test.go b/file_test.go index f9d709d..3cb2172 100644 --- a/file_test.go +++ b/file_test.go @@ -131,14 +131,14 @@ func TestParseSyntaxError(t *testing.T) { func TestParseLimits(t *testing.T) { var headers strings.Builder - for i := range 1 << 20 { + for i := range 1 << 16 { fmt.Fprintf(&headers, "[a \"%x\"]", i) } for _, tc := range []struct{ name, data string }{ - {"size", strings.Repeat("#\n", 8<<20+1)}, - {"entries", "[a]\n" + strings.Repeat("k\n", 1<<20)}, + {"size", strings.Repeat("#\n", 2<<20+1)}, + {"entries", "[a]\n" + strings.Repeat("k\n", 1<<16)}, {"distinct headers", headers.String() + "\n\tk\n"}, - {"items", strings.Repeat("\n", 4<<20+1)}, + {"items", strings.Repeat("\n", 1<<18+1)}, } { t.Run(tc.name, func(t *testing.T) { _, err := gitconfig.Parse("f", []byte(tc.data)) @@ -150,14 +150,14 @@ func TestParseLimits(t *testing.T) { } t.Run("at the limits", func(t *testing.T) { - _, err := gitconfig.Parse("f", []byte("[a]\n"+strings.Repeat("k\n", 1<<20-1))) + _, err := gitconfig.Parse("f", []byte("[a]\n"+strings.Repeat("k\n", 1<<16-1))) require.NoError(t, err) - _, err = gitconfig.Parse("f", []byte(strings.Repeat("\n", 4<<20))) + _, err = gitconfig.Parse("f", []byte(strings.Repeat("\n", 1<<18))) require.NoError(t, err) }) t.Run("an edit stays within the limits", func(t *testing.T) { - data := "[a]\n" + strings.Repeat("k\n", 1<<20-1) + data := "[a]\n" + strings.Repeat("k\n", 1<<16-1) f := mustParse(t, data) err := f.Add("a.k", "v") require.ErrorIs(t, err, gitconfig.ErrLoadLimit) @@ -1105,9 +1105,12 @@ func BenchmarkParse(b *testing.B) { {"comments", func(n int) []byte { return sized("", "# ;;;; [a] \\\n", n) }}, } for _, in := range inputs { - for _, n := range []int{1 << 10, 1 << 16, 1 << 20} { + for _, n := range []int{1 << 10, 1 << 16, 1 << 18} { data := in.gen(n) b.Run(fmt.Sprintf("%s/%dKiB", in.name, n>>10), func(b *testing.B) { + // Within the limits: an error would measure the wrong path. + _, err := gitconfig.Parse("f", data) + require.NoError(b, err) b.SetBytes(int64(len(data))) b.ReportAllocs() for b.Loop() { @@ -1140,7 +1143,7 @@ func BenchmarkEdit(b *testing.B) { }}, } for _, e := range edits { - for _, n := range []int{1 << 10, 1 << 16, 1 << 20} { + for _, n := range []int{1 << 10, 1 << 16, 1 << 18} { data := e.gen(n) b.Run(fmt.Sprintf("%s/%dKiB", e.name, n>>10), func(b *testing.B) { b.SetBytes(int64(len(data))) diff --git a/load.go b/load.go index bf2dad5..fd56c2c 100644 --- a/load.go +++ b/load.go @@ -20,13 +20,17 @@ import ( // Memory follows the entries, not the bytes: one entry ("k\n") can cost // a hundred times its size. Distinct section headers count as entries, as // the parser keeps each one. A parsed File also keeps every piece of the -// file (blank lines, comments, ...), at most maxFileItems of them. +// file (blank lines, comments, ...), at most maxFileItems of them: four per +// entry, as a line holds indentation, an entry, a comment and a newline. +// +// A typical configuration has tens to hundreds of entries; the limits leave +// room for one that tools have appended to for years. const ( maxIncludeDepth = 10 maxLoadFiles = 1000 - maxLoadBytes = 16 << 20 - maxLoadEntries = 1 << 20 - maxFileItems = 4 << 20 + maxLoadBytes = 4 << 20 + maxLoadEntries = 1 << 16 + maxFileItems = 4 * maxLoadEntries ) // repo is the repository of an Env.