| Message ID | 20220315105723.19398-3-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/4] terminal: use flags for save_term() | expand |
Phillip Wood <phillip.wood123@gmail.com> writes: > diff --git a/compat/terminal.c b/compat/terminal.c > index da2f788137..bfbde3c1af 100644 > --- a/compat/terminal.c > +++ b/compat/terminal.c > @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig) > static int term_fd = -1; The variable can be -1 to signal "no valid file descriptor". > static struct termios old_term; > > +static void close_term_fd(void) > +{ > + if (term_fd) > + close(term_fd); > + term_fd = -1; > +} > + And we use that negative value after closing it. The check before closing it is wrong. It should be if (0 <= term_fd) instead. Or we could mimick the beginning of restore_term(), i.e. if (term_fd < 0) return; close(term_fd); term_fd = -1; > void restore_term(void) > { > if (term_fd < 0) > return; > > tcsetattr(term_fd, TCSAFLUSH, &old_term); > - close(term_fd); > - term_fd = -1; > + close_term_fd(); Because we come this far only when term_fd is valid, this change is a no-op. If we are adding more calls to close_term_fd(), it may be a good abstraction. > sigchain_pop_common(); > } > > int save_term(enum save_term_flags flags) > { > if (term_fd < 0) > - term_fd = open("/dev/tty", O_RDWR); > + term_fd = (flags & SAVE_TERM_STDIN) ? 0 > + : open("/dev/tty", O_RDWR); We can avoid overly long line by wrapping differently: term_fd = ((flags & SAVE_TERM_STDIN) ? 0 : open("/dev/tty", O_RDWR)); We can turn our head sideways to see the parse tree this way. > @@ -66,8 +73,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits) > > sigchain_pop_common(); > error: > - close(term_fd); > - term_fd = -1; > + close_term_fd(); OK. > @@ -362,7 +368,7 @@ int read_key_without_echo(struct strbuf *buf) > static int warning_displayed; > int ch; > > - if (warning_displayed || enable_non_canonical(0) < 0) { > + if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) { > if (!warning_displayed) { > warning("reading single keystrokes not supported on " > "this platform; reading line instead"); The validity of this change is harder to see with only what we have in the patch, but the control flow is enable_non_canonical(bits) -> disable_bits(bits, ...) -> save_term(bits) And we are passing SAVE_TERM_STDIN to say "reuse fd #0 and save the terminal settings of it, instead of opening /dev/tty anew". What happens if FD#0 is *not* connected to a tty, by the way? tcgetattr() in save_term() would fail, without clearing term_fd (i.e. term_fd is still 0 when save_term() returns a failure), and goes to the error code path, where close_term_fd() is called. Because we have the "if (term_fd)" bug in save_term(), this bug is hidden, but I think save_term() upon seeing a failure from tcgetattr() should clear term_fd to limit the damage, especially when it is trying to futz with caller supplied FD#0, not the tty it opened itself? > diff --git a/compat/terminal.h b/compat/terminal.h > index aeb24c9478..79ed00cf61 100644 > --- a/compat/terminal.h > +++ b/compat/terminal.h > @@ -4,6 +4,8 @@ > enum save_term_flags { > /* Save input and output settings */ > SAVE_TERM_DUPLEX = 1 << 0, > + /* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */ > + SAVE_TERM_STDIN = 1 << 1, > }; > > /*
On March 15, 2022 1:43 PM, Junio C Hamano wrote: >Phillip Wood <phillip.wood123@gmail.com> writes: > >> diff --git a/compat/terminal.c b/compat/terminal.c index >> da2f788137..bfbde3c1af 100644 >> --- a/compat/terminal.c >> +++ b/compat/terminal.c >> @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig) >> static int term_fd = -1; > >The variable can be -1 to signal "no valid file descriptor". > >> static struct termios old_term; >> >> +static void close_term_fd(void) >> +{ >> + if (term_fd) >> + close(term_fd); >> + term_fd = -1; >> +} >> + > >And we use that negative value after closing it. > >The check before closing it is wrong. It should be > > if (0 <= term_fd) Should this expression succeed if term_fd == stdin? I might be missing the point here. --Randall
<rsbecker@nexbridge.com> writes: >>The check before closing it is wrong. It should be >> >> if (0 <= term_fd) > > Should this expression succeed if term_fd == stdin? I might be missing the point here. We could use "if (0 < term_fd)" to make this guard both about avoiding to call close() on an uninitialized FD and also about avoiding to close standard input. I'd prefer to see them handled separately as these live at different conceptual levels (i.e. closing -1 is a no-no no matter what, closing 0 is bad if it is what we did not open but what the caller supplied us via the SAVE_TERM_STDIN bit, but it may be warranted if it was what we obtained from an earlier call to open("/dev/tty") we did ourselves).
On March 15, 2022 3:05 PM, Junio C Hamano wrote: >To: rsbecker@nexbridge.com >Cc: 'Phillip Wood' <phillip.wood123@gmail.com>; 'Git Mailing List' ><git@vger.kernel.org>; 'Phillip Wood' <phillip.wood@dunelm.org.uk>; 'Ævar >Arnfjörð Bjarmason' <avarab@gmail.com>; 'Carlo Arenas' <carenas@gmail.com>; >'Johannes Schindelin' <Johannes.Schindelin@gmx.de>; 'Ramsay Jones' ><ramsay@ramsayjones.plus.com> >Subject: Re: [PATCH v3 2/4] terminal: don't assume stdin is /dev/tty > ><rsbecker@nexbridge.com> writes: > >>>The check before closing it is wrong. It should be >>> >>> if (0 <= term_fd) >> >> Should this expression succeed if term_fd == stdin? I might be missing the point >here. > >We could use "if (0 < term_fd)" to make this guard both about avoiding to call >close() on an uninitialized FD and also about avoiding to close standard input. I'd >prefer to see them handled separately as these live at different conceptual levels >(i.e. closing -1 is a no-no no matter what, closing 0 is bad if it is what we did not >open but what the caller supplied us via the SAVE_TERM_STDIN bit, but it may be >warranted if it was what we obtained from an earlier call to open("/dev/tty") we >did ourselves). Thanks. This is one of those situations where explaining magic numbers is important. I appreciate the insight. Regards, Randall
diff --git a/compat/terminal.c b/compat/terminal.c index da2f788137..bfbde3c1af 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -23,21 +23,28 @@ static void restore_term_on_signal(int sig) static int term_fd = -1; static struct termios old_term; +static void close_term_fd(void) +{ + if (term_fd) + close(term_fd); + term_fd = -1; +} + void restore_term(void) { if (term_fd < 0) return; tcsetattr(term_fd, TCSAFLUSH, &old_term); - close(term_fd); - term_fd = -1; + close_term_fd(); sigchain_pop_common(); } int save_term(enum save_term_flags flags) { if (term_fd < 0) - term_fd = open("/dev/tty", O_RDWR); + term_fd = (flags & SAVE_TERM_STDIN) ? 0 + : open("/dev/tty", O_RDWR); if (term_fd < 0) return -1; if (tcgetattr(term_fd, &old_term) < 0) @@ -66,8 +73,7 @@ static int disable_bits(enum save_term_flags flags, tcflag_t bits) sigchain_pop_common(); error: - close(term_fd); - term_fd = -1; + close_term_fd(); return -1; } @@ -362,7 +368,7 @@ int read_key_without_echo(struct strbuf *buf) static int warning_displayed; int ch; - if (warning_displayed || enable_non_canonical(0) < 0) { + if (warning_displayed || enable_non_canonical(SAVE_TERM_STDIN) < 0) { if (!warning_displayed) { warning("reading single keystrokes not supported on " "this platform; reading line instead"); diff --git a/compat/terminal.h b/compat/terminal.h index aeb24c9478..79ed00cf61 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -4,6 +4,8 @@ enum save_term_flags { /* Save input and output settings */ SAVE_TERM_DUPLEX = 1 << 0, + /* Save stdin rather than /dev/tty (fails if stdin is not a terminal) */ + SAVE_TERM_STDIN = 1 << 1, }; /*