Repository navigation
fix(gitlimit): keep only the tail of git's stderr in git errors - #759
Conversation
Run's errors start with the whole git command line, and they reach the server's log. Some argvs are request-sized: a grep pattern or a list of paths from the API has no length limit of its own, and past 128 KiB one argument fails exec with E2BIG. Each such call logged a record as large as the request. Run now quotes at most 4096 bytes of the command (maxErrArgvBytes), cut at a rune boundary, and then its full size: "git grep -e aaa... (204812 bytes)". A shorter command reads as before. TestRunErrorBoundsTheArgv runs a fake git with a 64 KiB argument (git exits 1) and a 200 KiB one (exec fails with E2BIG). Before this change the errors were 65,566 and 204,907 bytes long. Change-Id: I53303afd1147fd53ecc70260e40ccf2b1004dcd5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Commit and merge requests pass the commit message to git as an argument (commit-tree -m), and wcScratch.git's errors quoted the whole command line, so a message past 128 KiB (exec fails with E2BIG) logged a record as large as the message. The error now quotes the command through gitread.CommandLine (exported from Run's error path), which keeps 4096 bytes and the size. TestWCScratchGitErrorBoundsTheArgv runs commit-tree with a 200 KiB message. With the old error line the error was 204,932 bytes long. Change-Id: I7c4756a0fe24dd8cc527de23508dbd6ef2e3d2ba Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I85b7d4cf39b81008a060df7c67be0582a3c3ea47 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
applyPatch and wrefGit still quoted their whole command line, and wcScratch.git and wrefGit kept all of git's stderr in a bytes.Buffer. The API logs these errors, so a request-sized argument or a git that prints megabytes made a log record as large. All three helpers now quote the command through gitread.CommandLine, and wcScratch.git and wrefGit keep git's stderr in gitread.TailBuffer (exported from Run's tailBuffer), which keeps the last 64 KiB: git prints its fatal message last. The README lists both and states the bound. TestWriteGitErrorsBoundTheArgv runs the three helpers with a 200 KiB argument: before this change the applyPatch and wrefGit errors were 204,891 and 204,888 bytes. TestWriteGitErrorsKeepStderrTail runs a git that prints 1 MiB to stderr: both errors were 1,048,617 bytes. Change-Id: Ic40b87c486f6dc54fd6757d9e38c8634e877d7ba Signed-off-by: Thomas Kosiewski <tk@coder.com>
applyPatch's plain error (a cancelled request, a git killed by a signal) quoted all of git's stderr, which can echo the patch. It now keeps the last 64 KiB through gitread.TailBuffer. The classification below it still reads all of the stderr. TestCommandLineCut pins the cut on both sides: a 4096-byte command stays whole and a 4097-byte one is cut at 4096, so a cap of 1024, 5000 or 7000 fails. A 3-byte and a 4-byte rune across the cut need two and three steps back, so a loop limited to one or two steps fails. TestWriteGitErrorsKeepStderrTail gets an applyPatch case, a git apply that prints 1 MiB and dies of SIGTERM (1,048,631 bytes before). Change-Id: Idf2401f68e3b21eb46b92b1cdafcf4a7af243f35 Signed-off-by: Thomas Kosiewski <tk@coder.com>
githttp cannot import gitread (gitread imports githttp), and sync and lfsscan import neither, but their git runners need the same bounds. gitlimit, which already starts and reaps every git child, now holds CommandLine and TailBuffer. gitread.Run and the write-path helpers use them from there, and TestCommandLineCut moves with them. No behavior changes. Change-Id: Ie64ca578793f9a03bea4beeea1ca03d47df5cf92 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
TestTailBufferKeepsTheLast64KiB writes 40 chunks of about 5 KB and expects exactly the last 64 KiB. TestRunErrorKeepsTheStderrTail runs a git that prints 1 MiB to stderr and expects Run's error to end with exactly its last 64 KiB. Mutants that fail them: a tail of 1, 32, 60 or 128 KiB, and Run with a plain bytes.Buffer (a 1,048,618-byte error). TestWriteGitErrorsBoundTheArgv ran a real git with a 200 KiB argument and relied on exec failing with E2BIG, which only Linux guarantees: elsewhere git starts and can echo the argument into stderr. It now runs a fake git that prints nothing, with a 64 KiB argument (git runs and exits 1) and a 200 KiB one (E2BIG on Linux). Change-Id: I28575a148086c976a11a67c799d3ec1334926928 Signed-off-by: Thomas Kosiewski <tk@coder.com>
gitlimit.Name (each argument cut at 256 bytes, at most 32 arguments). This branch's gitlimit.CommandLine did the same job, so it goes, with TestCommandLineCut and TestRunErrorBoundsTheArgv. applyPatch's plain error, which #757 left on strings.Join, uses gitlimit.Name too, and TestApplyPatchErrorBoundsTheArgv keeps only that case. What stays from this branch: gitlimit.TailBuffer (the last 64 KiB of git's stderr) in Run, wcScratch.git, wrefGit and applyPatch's plain error, and its tests. Change-Id: If56daff93587005b6557293eab407a1bddec895a Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
gitread.Run, the write path's git helpers (wcScratch.git,wrefGit) andapplyPatch's plain error quote git's stderr in their errors, and the API logs those errors.wcScratch.gitandwrefGitkept all of git's stderr, andapplyPatch's plain error (a cancelled request, a git killed by a signal) quoted all of it, after its whole command line. A git that prints 1 MiB to stderr gave a log record of about 1 MB.#757 bounded the command line in
Run,wcScratch.gitandwrefGitwithgitlimit.Name. This PR bounds git's stderr in all four, andapplyPatch's command line.What changes
Runalready kept only the last 64 KiB of git's stderr. That buffer moves tointernal/gitlimitasgitlimit.TailBuffer, so git runners outside gitread can use it (githttp cannot import gitread).wcScratch.gitandwrefGitwrite git's stderr into it, andapplyPatch's plain error quotes its tail. git prints its fatal message last.applyPatch's plain error names its command withgitlimit.Name.internal/gitlimit/README.mdlistsTailBufferand its bound, andinternal/gitread/README.mdstates thatRun's errors keep it.The first rounds of this PR added
gitlimit.CommandLinefor the command line. #757 landedgitlimit.Namefor the same job first, so the merge of main removesCommandLineand its tests.Still unbounded after this PR
runGitCtx,localRefSeq,updateRefStdinand the commit-graph write, jobs'runGitStdin, sync'srunGitTo, the three children oflfsscan.ReachableLFSOIDs, and simtest'ssimGit, and the write path'swcBlobSink(hash-object -w). A follow-up PR (branchbounded-stderr-git-runners) moves them togitlimit.TailBuffer. It opens after this PR merges, because it uses it.POST /repos/commit-packanswers a failedhash-objectwith 400 and git's stderr in the body. fix(api): answer 404 or 400 for a git check only when git says no #760 answers 500 and logs the error instead.fix-long-git-args-client-input) fixes that input.Verification
TestWriteGitErrorsKeepStderrTailruns a git that prints 1 MiB to stderr. Before this PR thewcScratch.gitandwrefGiterrors were 1,048,617 bytes, and theapplyPatchplain error (a git apply that dies of SIGTERM) was 1,048,630 bytes.TestTailBufferKeepsTheLast64KiB(internal/gitlimit) writes 40 chunks of about 5 KB and expects exactly the last 64 KiB.TestRunErrorKeepsTheStderrTailruns a git that prints 1 MiB to stderr and expectsRun's error to end with exactly its last 64 KiB. Mutants that fail them: a tail of 1, 32, 60 or 128 KiB, andRunwith a plain buffer (a 1,048,618-byte error).TestApplyPatchErrorBoundsTheArgvrunsapplyPatchwith a fake git that prints nothing (a real git can echo the argument into stderr) and dies of SIGTERM, with a 64 KiB argument (git runs) and a 200 KiB one (exec fails with E2BIG on Linux). Withstrings.Jointhe error was 65,577 bytes.wcScratch.git,wrefGitorRunwith a plain buffer, andapplyPatchwithout its tail or back onstrings.Join.-race, and with-tags=openstorage_viewcheckexcept on the local backend, as in CI:internal/gitlimit,internal/gitread,internal/apiandcmd/openstoragepass on PostgreSQL, local and S3-WAL.internal/gitreadandinternal/gitlimittest binaries build for linux and darwin on amd64 and arm64. golangci-lint: 0 issues.go vet ./...is clean. Both--fingerprints-onlychecks pass, and the doc-link check finds 0 broken links.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh