clone 416976 -1 -2 -3
retitle 416976 top should really check for EOF
reassign -1 powertop
retitle -1 Should check for EOF
reassign -2 bash
retitle -2 Should not end job control on SIGHUP
reassign -3 ncurses
retitle -3 Should check for EOF/EIO
thanks
Armin Burchardt, le Wed 02 Jan 2008 10:59:08 +0100, a écrit :
On Wed, Jan 02, 2008 at 02:28:36AM +, Samuel Thibault wrote:
I can't reproduce it with procps 3.2.7-3, xterm 229-1 and ncurses
5.6+20071124-1.
I can reproduce this with unstable using
libncurses55.6+20071215-1
procps 1:3.2.7-5
xterm 229-1
Ok, managed to reproduce it:
xterm -e bash
su to root
top
and nicely close the xterm (close WM event, not kill WM event)
Doesn't happen when using kill WM event, doesn't happen with zsh,
doesn't happen with rxvt, doesn't happen without using a su :)
What happens is that when it gets nicely closed, xterm sends a SIGHUP
to the group of the user's bash, which then tries to forward it to su
and of course fails (EPERM), but ignores that and tcsetpgrp() to its own
group, so that when the pty actually gets closed, the kernel doesn't
send SIGHUP to the group of top, since it is not the foreground group
any more. Zsh for instance doesn't do this, and then it works fine.
It looks to me that a correct fix in bash is hence to not call
end_job_control in the SIGHUP handler, see attached patch.
Then, there is the problem of top spinning on EOF. It looks like
ncurses doesn't care at all about output errors and happily returns
OK. Then the main loop of top uses select() to wait for input, and that
returns immediately since xterm is gone and hence stdin is at EOF, but
neither ncurses or the main loop of top cares, thus spinning.
// EOF is pretty much a can't happen except for a kernel bug.
// We should quickly die via SIGHUP, and thus not spin here.
// if (rc == 0) end_pgm(0); /* EOF from terminal */
reveals to be wrong :) And actually it's quite easy to reproduce:
if (!fork()) execlp(top, top, NULL);
exit(0);
Since the parent leaves the group, top gets orphaned and as it is not a
session leader, won't get HUPed. Attached is a patch to fix top.
A friend of mine actually had exactly the same problem with powertop,
thus cloning and attached similar patch.
Concerning ncurses, I'm a bit uneasy. In an ideal world, it should
always check for EOF and EIO in puts, tput etc, but usually nobody
checks the result of these functions... Cloning anyway since the errors
should really get reported.
Samuel
--- sig.c.orig 2008-01-04 00:20:24.0 +
+++ sig.c 2008-01-04 00:20:35.0 +
@@ -436,7 +436,8 @@
#if defined (JOB_CONTROL)
if (interactive sig == SIGHUP)
hangup_all_jobs ();
- end_job_control ();
+ else
+end_job_control ();
#endif /* JOB_CONTROL */
#if defined (PROCESS_SUBSTITUTION)
--- top.c.orig 2008-01-03 23:04:26.0 +
+++ top.c 2008-01-03 23:09:16.0 +
@@ -3382,9 +3382,8 @@
// check 1st, in case tv zeroed (by sig handler) before it got set
rc = chin(0, c, 1);
if (rc = 0) {
-// EOF is pretty much a can't happen except for a kernel bug.
-// We should quickly die via SIGHUP, and thus not spin here.
-// if (rc == 0) end_pgm(0); /* EOF from terminal */
+if (rc == 0) end_pgm(0); /* EOF from terminal, may happen if top
+ * erroneously gets detached from it. */
fcntl(STDIN_FILENO, F_SETFL, file_flags);
select(1, fs, NULL, NULL, tv);
fcntl(STDIN_FILENO, F_SETFL, O_NONBLOCK|file_flags);
--- powertop.c.orig 2008-01-03 23:15:30.0 +
+++ powertop.c 2008-01-03 23:16:42.0 +
@@ -589,8 +589,11 @@
if (key) {
char keychar;
+ int key = fgetc(stdin);
+ if (key == EOF)
+ exit(EXIT_SUCCESS);
- keychar = toupper(fgetc(stdin));
+ keychar = toupper(key);
if (keychar == 'Q')
exit(EXIT_SUCCESS);
if (keychar == 'R')
