fix(tracers): detect TTY on /dev/pts/0 - #977
ANAMASGARD wants to merge 1 commit into
Conversation
Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change upgrades ChangesExec TTY metadata
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The TTY metadata update has no remaining actionable risk identified in the changed behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
There was a problem hiding this comment.
Review: request changes
Reviewed head 489d5e26a5fa81826b3f58afa2c8b3fd678726a5 against main (5acac56a).
The TTY fix itself works — Test_35_ExecTTYFieldTest passes on CI with the new gadget, and I confirmed locally that ig 0.48.1 can pull/export trace_exec:v0.55.0 and that TestExecFields passes against the pinned IG runtime (kubescape/inspektor-gadget@06b0d12, based on upstream ~v0.48.0). Bumping the gadget is the right way to close #975.
Blocker: the bump silently changes args semantics and breaks R0040 / existing profiles.
Between v0.48.1 and v0.55.0, trace_exec also picked up inspektor-gadget commits 06323f6d ("read args from the new process address space"), c759b4e1 ("report argv[0] instead of the exec path for failed execs") and 03c0f590 (lazy arg parsing). Net effect: args[0] used to be the pathname passed to execve (e.g. /bin/sh); it is now the committed argv[0] (e.g. sh). For anything launched through a shell or via PATH lookup those differ.
node-agent stores the full args vector (including args[0]) in container profiles and compares it verbatim in cp.was_executed_with_args (rule R0040), and utils.GetExecPathFromEvent returns args[0]. Consequences:
- CI already shows it:
component-tests (Test_32_UnexpectedProcessArguments)fails on this PR (run 35366394715, job 105671995732), while it passes on other recent PRs (e.g. runs 35364356926, 34937021951). Example:sh -c "echo hi"→ runtime argv[sh, -c, echo hi]no longer matches profile[/bin/sh, -c, ⋯⋯], so R0040 fires ("must stay silent", expected 2 got 3). The workflow is green only becausecomponent-testshascontinue-on-error: true. - Fleet impact: every already-learned/authored profile encodes the old
args[0]form, so after upgrade R0040 would alert on shell-launched execs across the board, and freshly learned profiles would be in a different format than existing ones (mixed-version fleets get both). pkg/malwaremanager/v1builds/proc/<pid>/root/<args[0]>viaGetHostFilePathFromEvent; with a relativeargv[0]that path no longer resolves. AlertCommandLine/execfields inruleadapters/adapters/exec.gochange shape too.
Suggested remedies (either is fine; the first is the smaller blast radius):
- Keep the gadget at v0.48.1 and vendor
trace_execunderpkg/ebpf/gadgets/with only upstreamc6fc831a(thetty_major/tty_minorchange) applied, built via the existingBUILT_GADGETSmechanism exactly liketrace_openalready is. - Stay on v0.55.0 but make the args change explicit: restore path-style
args[0](e.g. fromexepath) inexecoperator.goor adjust profile recording/matching to be robust to both forms, updateTest_32/fixtures, and document the profile-compatibility implication. This needs a real migration story for existing profiles, so it's more than a follow-up.
Non-blocking:
docs/features/exec-tty-field.md: "the regression fixed by #975" — #975 is the issue; this PR is the fix.Makefile: the trailingsudo ig image pullfortrace_execis fine, but consider folding it into the existing$(foreach …)with a per-gadget version map if more gadgets end up pinned separately.
Validation performed: go build ./... OK; go test ./pkg/utils ./pkg/rulemanager/cel/... ./pkg/rulemanager/types/v1 ./pkg/containerprofilemanager/... OK; TestExecFields OK against a locally exported trace_exec:v0.55.0; CI Test_35 pass / Test_32 fail as above. I did not run the component suite locally (needs kind).
Search notes: related history is #868 (introduced the two-phase TTY fields and Test_35), #695 (IG v0.48.1 upgrade), #800 (exec path recording symmetry — relevant to the args[0] convention). No prior PR attempts a trace_exec bump; nothing superseding this one.
|
|
||
| const ( | ||
| execImageName = "ghcr.io/inspektor-gadget/gadget/trace_exec:v0.48.1" | ||
| execImageName = "ghcr.io/inspektor-gadget/gadget/trace_exec:v0.55.0" |
There was a problem hiding this comment.
Blocker. trace_exec v0.55.0 also includes upstream 06323f6d/c759b4e1: args[0] changes from the execve pathname (/bin/sh) to the committed argv[0] (sh). node-agent records the full args vector in profiles and R0040 (cp.was_executed_with_args) compares it verbatim, so existing profiles stop matching — Test_32_UnexpectedProcessArguments already fails on this PR (job 105671995732) while passing on other current PRs. GetExecPathFromEvent/malware-manager /proc/<pid>/root/<args[0]> are affected too. Suggest vendoring trace_exec at v0.48.1 + only the tty commit (c6fc831a) via BUILT_GADGETS like trace_open, or explicitly restoring/handling the args[0] convention and its profile migration before bumping.
Related issues/PRs
hasTtyis false on /dev/pts/0: bump trace_exec gadget to v0.55.0+ #975Overview
trace_exec:v0.48.1reports only the driver-local TTY index. This makes/dev/pts/0indistinguishable from a process without a terminal because both reporttty=0, causingevent.hasTtyto incorrectly evaluate tofalse.This change pins only the
trace_execgadget tov0.55.0, which emits the terminal device major and minor numbers.After this change:
event.hasTtyisfalsefor processes without a TTY.event.hasTtyistruefor/dev/pts/0.event.hasTtyistruefor nonzero PTY indices.has(event.ttyMajor)is true for all emitted exec events, including no-TTY events where the major value is zero.The strict exec schema test, component rules, acceptance assertions, and documentation were updated accordingly. No public CEL fields or Go module dependencies were changed.
Additional Information
advise_seccomp,trace_capabilities,trace_dns, andtrace_openremain on their existing versions.trace_exec:v0.55.0:ctime,fctime,pctime,tty_major, andtty_minor.go.mod,go.sum, and workflows are unchanged.How to Test
Summary by CodeRabbit
Bug Fixes
/dev/pts/0and other pseudo-terminals.Enhancements
Documentation