Stop rs_getpass() from spinning forever on a closed stdin - #381
Merged
Conversation
`getch()` forwards `getchar()`, whose EOF is -1. The POSIX rs_getpass() stored it in an `unsigned char`, where it becomes 0xFF and never equals RETURN, so a closed stdin made the read loop append one 0xFF byte per iteration for good. Measured on retroshare-service built from master, started as `-U <sslid> < /dev/null`: 23 MB of output in 90 seconds, one core at 100%, and the process does not answer SIGTERM -- the spin never returns to the shutdown check, so only SIGKILL ends it. That is the behaviour a service unit or a container gets today whenever the passphrase is asked with no terminal attached, which also covers stdin redirected from /dev/null and an ssh session dropping while the prompt is up. `ch` is now an int and EOF ends the read with an empty result: the same run then exits in under a second with a 2.5 kB log. The Windows branch had the milder form of the same bug: a closed console returns EOF from every `_getch()`, and the loop could only leave through the PASS_MAX branch, so it returned 512 bytes of 0xFF as if they had been typed. It now stops on EOF as well, and accepts '\n' next to '\r' so a redirected stdin terminates the line. An optional `eof` out parameter tells the caller the input stream ended, which a plain empty string cannot express: without it, a caller looping "passphrase cannot be empty, try again" spins just as hard one level up. It defaults to nullptr, so existing calls are unchanged. Also check tcgetattr(): it fails whenever stdin is not a terminal, and t_old was then applied to stdin uninitialized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getch()forwardsgetchar(), whose EOF is -1.rs_getpass()stored it in anunsigned char, where it becomes0xFFand never equalsRETURN, so a closed stdin makes the read loop append one0xFFbyte per iteration for good.Measured on
retroshare-servicebuilt from current master, run as-U <sslid> < /dev/null:That is what a systemd unit or a container gets today whenever the passphrase is asked with no terminal attached. With the fix the same run exits in under a second with a 2.5 kB log.
Changes:
chbecomes anint, EOF ends the read with an empty result._getch()and the loop could only leave through thePASS_MAXbranch, returning 512 bytes of0xFFas if they had been typed. It now stops on EOF too, and accepts\nnext to\rso a redirected stdin terminates the line.eofout parameter, defaulting tonullptrso existing calls are unchanged. An empty string cannot distinguish "nothing typed" from "stream closed", and without that a caller looping "passphrase cannot be empty, try again" spins just as hard one level up.tcgetattr()return value checked: it fails whenever stdin is not a terminal, andt_oldwas then applied to stdin uninitialized.Built and run on Linux (CMake+Ninja, Qt5). The only caller is
retroshare-service; RetroShare/RetroShare#3290 makes that path much easier to reach, and a companion PR there bounds the prompt loops on top of this.