Skip to content

nshlib: fix infinite loop on broken stdout in nsh_catfile#3496

Open
JianyuWang0623 wants to merge 1 commit into
apache:masterfrom
JianyuWang0623:fix/nshlib-catfile-infinite-loop
Open

nshlib: fix infinite loop on broken stdout in nsh_catfile#3496
JianyuWang0623 wants to merge 1 commit into
apache:masterfrom
JianyuWang0623:fix/nshlib-catfile-infinite-loop

Conversation

@JianyuWang0623
Copy link
Copy Markdown
Contributor

Summary

Fixes an infinite loop in nsh_catfile (used by cat / dmesg) when stdout is broken — for example when a PTY master is closed while the shell is dumping /dev/log.

Failure mode:

  1. nsh_catfile reads a chunk from the input file (e.g. /dev/log).
  2. nsh_consolewrite calls write(OUTFD, ...), which fails (EPIPE/EIO/ENOSPC/…).
  3. The old code logged the failure via _err(), which the syslog backend turned into more bytes on /dev/log.
  4. nsh_catfile ignored the write error and looped back to read(), picking up the bytes that step 3 just produced.
  5. Goto 2 — the shell spins forever at the highest priority.

Two independent fixes, either of which would break the loop, both applied for defense in depth:

  • nshlib/nsh_console.c: drop the _err() in nsh_consolewrite entirely. The hazard is errno-agnostic: any logging at this layer can be re-injected when OUTFD is bound to the syslog backend. Callers already get the failure via the negative return value and errno, so on-failure logging here is redundant.
  • nshlib/nsh_fsutils.c: on inner-loop write failure in nsh_catfile, jump straight to a single errout: cleanup label instead of using a two-level break and falling through to another read(). A failed write can no longer be followed by another read of the same fd.

Impact

  • User-visible behavior: cat /dev/log, dmesg, and similar commands now exit cleanly with ERROR when stdout is broken, instead of hanging the shell. No change on the success path or for non-syslog outputs.
  • Logging: write failures inside nsh_consolewrite are no longer logged at this layer. The information is still available to callers through the return value and errno, and other layers (e.g. command implementations) remain free to report errors as appropriate.
  • API / build / config: none. No public API, Kconfig, or build changes.
  • Security: removes a denial-of-service style hang triggered by closing the controlling PTY while NSH is streaming the syslog device.
  • Compatibility: backward compatible. Behavior only changes on the previously broken error path.
  • Documentation: no doc changes required.

Testing

Regression checks on the happy path:

  • cat <regular file> — unchanged output, exit status 0.
  • dmesg to a healthy console — unchanged output.
  • cat of a non-existent / unreadable file — still reports the original error via the existing nsh_error() paths in callers; no behavioral change beyond the dropped _err() line in nsh_consolewrite.

When stdout is broken (e.g. PTY master closed), nsh_catfile could spin
forever: write() fails, error logging generates syslog output to
/dev/log, next read() picks it up, write() fails again, ad infinitum.

Two fixes:
- nsh_console: drop the _err() in nsh_consolewrite entirely.  The risk
  is errno-agnostic (EPIPE / EIO / ENOSPC / ...): any failure logged
  here can be re-injected by the syslog backend when OUTFD is bound to
  /dev/log.  Callers already see the failure via the negative return
  value and errno, so on-failure logging at this layer is redundant.
- nsh_fsutils: jump straight from the inner write-failure path to a
  single errout: cleanup label instead of using a two-level break +
  flag check, so a failed write cannot fall through to another read().

Assisted-by: GitHubCopilot:claude-4.7-opus
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants