lib/tty: add proper error handling for tcgetattr() by mukhanov · Pull Request #5173 · MidnightCommander/mc

mukhanov · 2026-10-03T14:57:28Z

Check if stdin is a terminal before calling tcgetattr()
Verify return value of tcgetattr() and handle errors properly
Exit with clear error message when stdin is not a terminal
Prevents mc from hanging when run in non-interactive environments

This fixes the issue where mc would hang on startup when stdin is not
a terminal (e.g., when run in background or in certain terminal emulators
on macOS). The program now exits with a clear error message instead of
hanging or exhibiting undefined behavior.

mc-worker · 2026-10-03T15:51:50Z

Treating !isatty (fileno (stdin)) as an error makes impossible an implementation of #2370 and #2629 in the future.

Btw, tty_init() in the lib/tty/tty-ncurses.c calls tcgetattr() without return value checking too.

mukhanov · 2026-10-03T22:08:25Z

Thank you for the feedback!

I've updated the fix to:

Only treat !isatty() as an error for MC_RUN_FULL mode
For MC_RUN_VIEWER and MC_RUN_EDITOR modes, tcgetattr() failures are handled gracefully by initializing the mode structure with default values
Also fixed the same issue in lib/tty/tty-ncurses.c

This approach:

Prevents mc from hanging when run in non-interactive environments (the original issue)
Allows future implementation of Allow mcview to display from stdin #2370 and Allow editor to read data from stdin #2629 (cat file | mcview/mcedit)
Properly handles tcgetattr() errors in both slang and ncurses backends

The changes ensure that viewer/editor modes can work with pipes while full mc mode still requires a proper terminal.

zyv · 2026-10-04T16:17:39Z

@mukhanov - thanks for your submission. I've screened it through Opus 5.5, and it has identified a number of issues and suggests a different approach. Would you be up to reviewing and testing the suggested alternative? I can submit the results later this coming week.

mukhanov · 2026-10-04T20:08:08Z

Hi @zyv,

Yes, I'd be happy to review and test the alternative approach suggested by Opus 5.5.

This is indeed a rather annoying bug - mc hanging on startup when stdin is not a terminal is a real problem that needs a proper fix. I'm particularly interested in seeing how Opus 5.5 handles the error cases differently from my initial approach.

Looking forward to reviewing the results when you have them ready.

Best regards

zyv · 2026-10-10T16:15:56Z

Okay, I have finally managed to process the alternative patch series generated with Opus 5.5, which do make sense to me. It includes a test that passes with the suggested changes and fails with your approach, and I think it addresses both points by @mc-worker. I've added the edited LLM review summary below for the details. Appreciate it if you could review and test it @mukhanov and report back.

The underlying problem is real, but the PR addresses it at the wrong level, and every run test reproduces at least one damaging behaviour:

It breaks a working use case on the default S-Lang backend. mc < /dev/null and ... | xargs mc work on master and now fail (F1).
It leaves the user's terminal without echo or line editing when the new error exits are taken on ncurses (F2).
It does not fix the hang it describes for mcview / mcedit / mcdiff on ncurses. They still spin at 100 % CPU on EOF or ignore the keyboard on a pipe (F3).
It codifies the zero-filled boot_mode as "default values". That mode is applied to the real terminal before every external command: speed 0, no ISIG (Ctrl-C cannot stop the command), no echo (F4, pre-existing).

Right fix: mc never reads data from stdin, but it reads the keyboard from stdin with ncurses, and both backends save the initial terminal modes from stdin. So the correct fix is to attach a redirected stdin to the controlling terminal once, in main(), before any terminal set-up, as xargs -o does ¹. Fall back to stderr as S-Lang does ², and fail with a clear message only when there is no terminal at all. This:

fixes the hang on both backends;
keeps the S-Lang use cases working;
fixes the zeroed boot_mode;
keeps stdin reading possible - that feature would dup() the original stdin first:
- Allow mcview to display from stdin #2370
- Allow editor to read data from stdin #2629

#	Severity	Finding
F1	High	FULL mode now refuses to start with redirected stdin on S-Lang (the default backend), although S-Lang reads keys from `/dev/tty` and this worked
F2	High	Zero-filled `boot_mode` is applied to the real terminal before every external command (speed 0, `-isig -icanon -echo -opost`). Pre-existing, but the PR rewrites this code and documents it as safe. Run test (`stty -a` from user menu).
F3	High	The hang the PR targets remains for viewer, editor and diff viewer on ncurses (100 % CPU on EOF, keyboard ignored on a pipe). The "stdin may be a pipe, e.g. `cat file \| mcview`" premise is not a feature mc has
F4	Medium	ncurses: both new `exit()` paths run after `initscr()` without `endwin()`, leaving the shell with `-echo -icanon -onlcr`
F5	Low	After a failed `tcgetattr()` the zero-filled struct is still written with `tcsetattr()` (ncurses). The S-Lang `new_mode` check tests the wrong call: `SLang_init_tty()`'s result is ignored
F6	Low	`exit()` from inside `tty_init()` bypasses `main()`'s startup cleanup and error format ("Failed to run: ...")
F7	Low	Same 15-line block pasted 3 times; two new translatable strings in a non-standard format; redundant `#include <string.h>`; `errno` used without `<errno.h>`

GNU findutils 4.9.0 xargs --help output (-o, --open-tty) and local run (echo x | xargs sh -c 'readlink /proc/$$/fd/0' → /dev/null). Local tool output, Ubuntu 24.04 container. ↩
S-Lang 2.3.3 source, src/slutty.c (SLang_init_tty(), lines 276-321). Ubuntu archive upstream tarball. ↩

zyv · 2026-10-10T16:18:16Z

P.S. I can look into the Solaris issue when everybody is satisfied with the approach in principle.

mc-worker · 2026-10-11T12:35:08Z

STDIN_FILENO and fileno (stdin) both are used in the MC codebase. Should we get rid of such mix and use only one of them?

zyv · 2026-10-11T13:20:09Z

STDIN_FILENO and fileno (stdin) both are used in the MC codebase. Should we get rid of such mix and use only one of them?

This is a good point; all I could find in the standard is this:

https://pubs.opengroup.org/onlinepubs/9799919799/functions/stdin.html

So, apparently, at least when the program starts, STDIN_FILENO == fileno (stdin), and two ways of doing this were introduced for convenience, because sometimes you need the descriptor, and sometimes you need the corresponding FILE * pointer. However, not even this invariant seems to hold in practice:

https://lists.rtems.org/pipermail/users/2011-March/023277.html

So the rule can probably be formulated like this: "if you haven't done anything on the descriptors yet, you can use STDIN_FILENO; otherwise, if you manipulated them with something like freopen and you need a descriptor, you must recover it with fileno(stdin)".

I think that in theory, we could change all usages of STDIN_FILENO to fileno (stdin) and this change would be equivalent in intent (but not necessarily equivalent in practice - see RTEMS example above). The opposite change of swapping all calls of fileno (stdin) for STDIN_FILENO will be incorrect.

My guess is that the previous policy was to use one or the other "as appropriate" as one "know what one's doing", and later, probably just copying what the last used one pattern was.

In this case, I and the LLM simply followed the current usage patterns without evaluating whether this is right or wrong. To my mind, the best style would be to use each "as appropriate", but I guess this is not achievable, since for that one has to understand what rebinds which descriptors where, how and why globally.

Therefore, I wouldn't object to you applying a radical solution moving everything to fileno (stdin) everywhere if this bothers you, but I think that this should be done in a separate branch / PR.

ossilator · 2026-10-11T16:00:07Z

i think using STDIN_FILENO throughout is just fine. mc doesn't freopen(stdin) (why would it?). also, sane libcs keep the fd constant (ex 1, ex 2) for exactly this case.

zyv · 2026-10-11T18:18:35Z

i think using STDIN_FILENO throughout is just fine. mc doesn't freopen(stdin) (why would it?). also, sane libcs keep the fd constant (ex 1, ex 2) for exactly this case.

Okay, this is very useful to know, thank you. In this case, I guess, we can decide on the mechanical migration to STDIN_FILENO and be prepared to fix it if this ever causes trouble. I have added it as a separate commit.

mukhanov · 2026-10-12T12:50:06Z

@zyv I have reviewed and tested the alternative patch series (fe3b6e1) on macOS/arm64, both S-Lang and ncurses builds. All 70 unit tests pass on each backend, including the new tty_stdin_to_terminal() tests. Verified manually: mc < /dev/null starts with working keyboard and idle CPU on both backends (the ncurses busy-loop is gone); cat | xargs mcview/mcedit work; xargs mc still works on S-Lang; running without any terminal exits 1 with the clear new message; terminal modes after exit and in the subshell are fully sane (echo, icanon, isig, opost on, speed 9600, Ctrl-C interrupts). The approach LGTM.

Three minor notes: (1) tcgetattr (SLang_TT_Read_FD, &new_mode) in tty-slang.c is still unchecked — probably fine since it can't realistically fail after SLang_init_tty() succeeds, but worth a comment or a check for consistency; (2) unused #include <termios.h> in tests/lib/tty.c; (3) the dup (STDERR_FILENO) fallback doesn't verify read access (write-only stderr like mc 2>/dev/tty < /dev/null would produce an unreadable stdin) — same as S-Lang does, so probably acceptable, just wanted it on record. None of these block the merge for me.

zyv

@mukhanov thanks for your notes and testing! I've finally gotten around to fixing the remaining issues. The Solaris issue was a test problem, and the rest of the notes should also be addressed now just for completeness. I've also fixed the outdated comment while I was at it.

ossilator · 2026-10-19T08:46:58Z

care to push a re-cut, yury? i'd review this from scratch, so the current state is not helpful.

zyv · 2026-10-19T09:24:33Z

/rebase

MC reads the keyboard from stdin with the ncurses backend, and both backends save the initial terminal modes from stdin. When stdin is redirected (e.g. "mc < /dev/null" or "find ... | xargs mcedit", GNU xargs gives the command /dev/null as stdin): * the ncurses build busy-loops on EOF, or blocks on a pipe, and does not react to the keyboard at all; * the S-Lang build reads the keyboard from /dev/tty, but saves the "shell" terminal modes from stdin, which fails and leaves them zero-filled, so every external command is run on a terminal with speed 0 and no echo, signals or output post-processing. Before initializing the terminal, attach a redirected stdin to the controlling terminal, like "xargs -o" does (fall back to stderr like S-Lang does if there is no controlling terminal), and fail with a clear message only if there is no terminal at all. MC never reads data from stdin; a future implementation of reading the viewer data from stdin (MidnightCommander#2370) will have to dup() the original stdin before this call. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

Run tty_stdin_to_terminal() in a new session on a fresh pseudo-terminal and check that a key typed on that terminal can be read from stdin when stdin is a terminal, when it is redirected and there is a controlling terminal, and when it is redirected and only stderr is a terminal, and that a failure is reported with stdin left untouched if the only terminal is a write-only stderr or there is no terminal at all. On System V systems, the pseudo-terminal is not a terminal until the ptem and ldterm STREAMS modules are pushed onto it, unless they are autopushed. Open it before setsid(), because on illumos pushing them in a session leader makes it the controlling terminal even with O_NOCTTY. Acquire the controlling terminal by opening it without O_NOCTTY first, and with TIOCSCTTY only if that did not work, because on illumos (and likely on Solaris) TIOCSCTTY fails if the terminal already is the controlling one. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

tty_init() ignored the result of tcgetattr() in both backends: * ncurses: an uninitialized structure was applied to the terminal with tcsetattr(). Only change VINTR/VQUIT if the modes were read. * S-Lang: boot_mode stayed zero-filled and was applied to the terminal before running every external command (speed 0, no echo, no signals, no output post-processing). Fail early instead, before the terminal is touched, like for an unsupported screen size. Also fail if SLang_init_tty() does, instead of running on a terminal that S-Lang has not set up. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

Also fix the comment in tty_change_screen_size(), which has referred to stdin since the ioctl() was switched to stdout in ea2c57d. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

zyv · 2026-10-19T09:30:58Z

care to push a re-cut, yury? i'd review this from scratch, so the current state is not helpful.

I just triggered an autorebase; I can drop the last (Solaris CI) commit and squash 59f5683 and/or remove the future direction speculations from 97e47b7 before merging if wanted.

ossilator

commit 1: the wording "attach redirected stdin" is just weird. it's also kinda imprecise, as one could redirect from another tty. the summary should be more like "ensure that stdin is a tty", and the complementary phrase "when stdin is not a tty [due to having been redirected]".
the speculation about future direction should stay.

commit 2 should be arguably squashed into commit 1 (in most projects, committing the fix and its regression test together is preferred/required).
in either case its commit message is excessive due to being redundant with the code comments (also, the commit message should only answer what+why, while the how belongs into comments).

why/where would you squash commit 3? it seems semantically separate to me.

it would be nice to pull commit 4 ahead to have a cleaner baseline, but that would require resolving merge conflicts.

commit 5's description should point out how this differs from other targets, implicitly answering why they don't need adjustment.

ossilator · 2026-10-19T11:39:04Z

+            return FALSE;
+    }
+
+    return isatty (STDIN_FILENO) != 0;


that check makes no sense at this point.

ossilator · 2026-10-19T11:41:31Z

+    {
+        mc_propagate_error (&mcerror, 0, "%s",
+                            _ ("Standard input is not a terminal and there is no controlling "
+                               "terminal to read the keyboard from"));


use "keyboard input" rather than just "keyboard" (also in the commit message and comment above).

ossilator · 2026-10-19T11:59:32Z

+    // given: stdin is a terminal
+
+    // when
+    const int actual_result = run_stdin_to_terminal (STDIN_IS_TTY);


doesn't the framework provide a way to do table-driven testing? the duplication of the bodies isn't very elegant.

ossilator · 2026-10-19T12:10:23Z

-    // disable SIGQUIT to allow use Ctrl-\ key
-    mode.c_cc[VQUIT] = NULL_VALUE;
-    tcsetattr (STDIN_FILENO, TCSANOW, &mode);
+    // don't apply an uninitialized structure if the terminal modes cannot be read


that's a rather pointless comment, because it's stating the obvious.
more interesting questions would be why it's bad if that happens and why it's ok to just skip over it. by extension this means pointing out why it would fail in the first place.

Builds run inside a VM, and workspace files are not available to the artifact upload step upon failure under the default sync strategy (rsync on success only). Switch to the nfs strategy establishing a live bidirectional workspace bridge between the host and the VM. Made possible by vmactions/solaris-vm#70. Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

zyv · 2026-10-22T06:58:19Z

commit 5's description should point out how this differs from other targets, implicitly answering why they don't need adjustment.

Oh, this is a long and painful story to tell. The sort of stuff most people use without willing to understand how it works, and then the tool sucks, obviously.

As I was setting up the CI during my full-time mc work streak, I found that the best way to test on "exotic" systems that are not directly supported by GitHub Actions is the vmactions system, running them in QEMU. Most Linux distributions are "easily" testable in Docker on top of the normal Actions runners, but not BSD or Solaris. The problem is that Docker shares the workspace with the runner, but QEMU is a completely separate machine, and the workspace is synced with rsync. Back then, there were no other possibilities as far as I remember, so if the build fails, the only possibility to peek into the VM was to print more.

Now, the author introduced NFS-backed shared workspaces, so in theory, even if the run fails, the changes are mirrored in the workspace. I tried it, and it wasn't working correctly on Solaris, even though it kind of worked on BSD. Luckily, the author is very responsive and has already fixed it in vmactions/solaris-vm#70. I've moved this to #5193.

ossilator · 2026-10-22T08:38:48Z

ok, i'd prepend something along the lines of "unlike the linux builds which are docker-based, the bsd and solaris builds have to use proper vms. unfortunately, ..."

pedantically, that commit also doesn't belong into this PR, but into a separate one (that would be merged first). it doesn't matter at the commit level, but the project policy is to use --no-ff, so correct logical grouping is preferable.

zyv · 2026-10-22T09:22:46Z

ok, i'd prepend something along the lines of "unlike the linux builds which are docker-based, the bsd and solaris builds have to use proper vms. unfortunately, ..."

To my mind, that's below the threshold of assumed knowledge for someone editing GitHub Actions workflows. The commit now says what makes these workflows different, and I wouldn't want to add more explanations as to how the rest of the system works and why. Besides, this is incorrect - the Linux builds are not Docker-based per se, and macOS/Windows builds are handled in a different fashion but natively.

pedantically, that commit also doesn't belong into this PR, but into a separate one (that would be merged first). it doesn't matter at the commit level, but the project policy is to use --no-ff, so correct logical grouping is preferable.

I've already replied earlier that I've moved it to a different branch. I didn't force-push here as I didn't get to the rest of the comments.

mukhanov · 2026-10-22T17:22:22Z

Hi @zyv, thanks again for working through this!

I see that the remaining review comments are being addressed. Happy to help with any of them or re-test the updated patch series on macOS (both S-Lang and ncurses) once it's ready.

Just let me know if there's anything you'd like me to take care of.

ossilator · 2026-10-23T09:18:18Z

To my mind, that's below the threshold of assumed knowledge for someone editing GitHub Actions workflows.

it certainly is, but you shouldn't assume such a specialized audience. providing some minimal context for newcomers and downstreams is worthwhile, even beyond the review phase.

I've already replied earlier that I've moved it to a different branch.

no, you said you moved it to an issue. with no visible artifacts, i was a bit confused. when you withhold work for practical reasons, it makes sense to be a tiny bit more verbose, say "already moved it to #...'s branch locally".

github-actions Bot added needs triage Needs triage by maintainers prio: medium Has the potential to affect progress labels Oct 3, 2026

github-actions Bot added this to the Future Releases milestone Oct 3, 2026

mukhanov force-pushed the fix/tcgetattr-error-handling branch from a1ffbc2 to d08e61a Compare October 3, 2026 22:08

zyv added area: tty Interaction with the terminal, screen libraries and removed needs triage Needs triage by maintainers labels Oct 4, 2026

zyv force-pushed the fix/tcgetattr-error-handling branch from 94f0396 to bf20399 Compare October 10, 2026 15:59

zyv modified the milestones: Future Releases, 4.9.0 Oct 15, 2026

zyv approved these changes Oct 15, 2026

View reviewed changes

zyv added 4 commits October 19, 2026 09:24

maint: consistently use STD*_FILENO instead of fileno (std*)

6898210

Also fix the comment in tty_change_screen_size(), which has referred to stdin since the ioctl() was switched to stdout in ea2c57d. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

mc-butler force-pushed the fix/tcgetattr-error-handling branch from 9c7c485 to 6f03713 Compare October 19, 2026 09:24

ossilator suggested changes Oct 19, 2026

View reviewed changes

zyv force-pushed the fix/tcgetattr-error-handling branch 2 times, most recently from 287b444 to 6f172fd Compare October 19, 2026 17:54

zyv force-pushed the fix/tcgetattr-error-handling branch from 6f172fd to 3d268ad Compare October 20, 2026 10:24

Conversation

mukhanov commented Oct 3, 2026

Uh oh!

mc-worker commented Oct 3, 2026

Uh oh!

mukhanov commented Oct 3, 2026

Uh oh!

zyv commented Oct 4, 2026

Uh oh!

mukhanov commented Oct 4, 2026

Uh oh!

zyv commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

zyv commented Oct 10, 2026

Uh oh!

mc-worker commented Oct 11, 2026

Uh oh!

zyv commented Oct 11, 2026

Uh oh!

ossilator commented Oct 11, 2026

Uh oh!

zyv commented Oct 11, 2026

Uh oh!

mukhanov commented Oct 12, 2026

Uh oh!

zyv left a comment

Choose a reason for hiding this comment

Uh oh!

ossilator commented Oct 19, 2026

Uh oh!

zyv commented Oct 19, 2026

Uh oh!

zyv commented Oct 19, 2026

Uh oh!

ossilator left a comment

Choose a reason for hiding this comment

Uh oh!

ossilator Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

ossilator Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

ossilator Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

ossilator Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

zyv commented Oct 22, 2026

Uh oh!

ossilator commented Oct 22, 2026

Uh oh!

zyv commented Oct 22, 2026

Uh oh!

mukhanov commented Oct 22, 2026

Uh oh!

ossilator commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

zyv commented Oct 10, 2026 •

edited

Loading