Re-implement cat builtin with full flag support#22
Re-implement cat builtin with full flag support#22gh-worker-dd-mergequeue-cf854d[bot] merged 4 commits intomainfrom
Conversation
Co-authored-by: AlexandreYang <49917914+AlexandreYang@users.noreply.github.com>
|
Bits Dev status: ✅ Done Comment @DataDog to request changes |
…device names Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
1 similar comment
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 413f8cb3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| flagE := fs.BoolP("show-nonprinting-ends", "e", false, "equivalent to -vE") | ||
| flagT := fs.BoolP("show-nonprinting-tabs", "t", false, "equivalent to -vT") | ||
| _ = fs.BoolP("unbuffered", "u", false, "ignored") |
There was a problem hiding this comment.
GNU cat --help documents -e, -t, and -u as short-only switches (no long forms), and GNU rejects --unbuffered/similar names as unrecognized options. Defining these with BoolP and custom long names here makes rshell accept --show-nonprinting-ends, --show-nonprinting-tabs, and --unbuffered, so invocations that should fail now succeed, which breaks the stated GNU-compatibility behavior and can hide mistyped flags.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,9 @@ | |||
| description: cat rejects unknown flags with exit code 1. | |||
| skip_assert_against_bash: true | |||
There was a problem hiding this comment.
AGENTS.md requires skip_assert_against_bash: true only for intentional divergences, but unknown flag handling should match bash/coreutils (cat --follow ... exits non-zero with an option error). Marking this scenario as skipped removes the bash parity check for a normal compatibility case, so parser regressions for unsupported flags can slip through unnoticed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 413f8cb3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| content, term := splitTerminator(line) | ||
| hasTerm := len(term) > 0 | ||
| blank := len(content) == 0 && hasTerm |
There was a problem hiding this comment.
cat is specified to concatenate inputs into one output stream (see cat --help: “Concatenate FILE(s) to standard output”), but this token handling treats each file’s first scanner token as a fresh line. If one file ends without \n, the next file’s first bytes are a continuation of that same logical line; with -n/-b this code will incorrectly add a new line number (e.g. a.txt="a", b.txt="b\n" should produce one numbered line ab\n, not two numbered fragments), and similar boundary errors also affect -s blank-line squeezing.
Useful? React with 👍 / 👎.
| if ctx.Err() != nil { | ||
| break |
There was a problem hiding this comment.
Breaking out of the file loop when ctx.Err() != nil returns success unless another file error occurred, which can silently truncate output with exit code 0 if cancellation/deadline happens between files. This regresses reliability for timeout-driven runs because callers can observe a successful cat even though not all requested files were processed.
Useful? React with 👍 / 👎.
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Security Audit: APPROVED
Overall Risk: LOW — 0 Critical, 0 High, 1 Medium (theoretical), 2 Low, 2 Informational
Summary
Performed a thorough security audit of the cat builtin reimplementation covering memory safety, sandbox bypass, DoS/resource exhaustion, context cancellation, integer overflow, flag parsing, byte handling, and stdin edge cases.
Findings
MEDIUM (theoretical) — lineNum (int64) could overflow after ~9.2×10¹⁸ lines (~8 EiB), producing 0 line numbers. Not practically reachable given context cancellation and shell timeouts. No crash or memory corruption.
All other areas clean:
- Memory: fixed 32 KiB buffer in
catRaw, 1 MiB scanner cap incatLines— no unbounded allocations - Sandbox: all file access delegated to
callCtx.OpenFile(), no filesystem ops in the builtin - DoS:
ctx.Err()checked every iteration in both code paths appendNonprinting: all 256 byte values covered, matches GNUcat -v- Flag parsing: unknown flags properly rejected via
pflag.ContinueOnError - Scanner split function: correct advance/token pairs, no infinite loop possible
Positive observations
- Strong streaming architecture — never loads full files into memory
- Clean sandbox delegation — zero filesystem logic in the builtin
- Comprehensive pentest test coverage (path traversal, boundary line caps, cancellation, CRLF)
- Proper error handling across multiple files
Well-engineered implementation following sound security principles for a restricted shell.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
dd/reimpl-cat-builtin-flags
branch
2 participants
What does this PR do?
Re-implements the
catbuiltin command with full GNU-compatible flag support. The previous implementation was bare-bones (no flags, unboundedio.Copy). The new version adds 11 flags, streaming line-by-line processing with bounded buffers, context cancellation, and proper error handling.Motivation
The existing
catbuiltin lacked flag support (-n,-b,-s,-v,-E,-T,-A,-e,-t,-u,-h) and used unboundedio.Copywhich is unsafe for infinite sources. This re-implementation brings it to parity with GNU coreutils cat while maintaining the safety guarantees required by RULES.md.Testing
cat_test.go): 33 tests covering all flags, edge cases, error paths, and RULES.md compliancecat_gnu_compat_test.go): 20 tests with byte-for-byte output verification against GNU coreutilsbuiltin_cat_pentest_test.go): 19 tests covering flag injection, path traversal, long lines at buffer cap, resource exhaustion, context cancellation, and CRLF edge casestests/scenarios/cmd/cat/covering number, number-nonblank, squeeze, show-ends, show-tabs, combined flags, help, and hardeningChecklist
PR by Bits
View session in Datadog
Comment @DataDog to request changes