[Pkg-kbd-devel] A bug in the openvt program from the kbd-1.15.2 package
Alexander Kozhushkin
alex at grave.users.botik.ru
Fri Apr 15 02:22:46 UTC 2011
Dear developers and maintainers of the kbd package.
I have found a bug in the openvt program from the recent kbd-1.15.2
package. The only likely reason for the bug is the possibility of a
critical race condition which has been introduced over development
of the utility. That race condition appears whenever the program is
started by the `init' process from `/etc/inittab' on a user
keyboard request (usually pressing the <left-Alt>+<UpArrow> keys
simultaneously). The necessary explanation and reasonable simple
solutions to the problem are given.
The Symptoms.
When I start the openvt from `/etc/inittab' by pressing
<left-Alt>+<UpArrow>, the program opens a new virtual terminal, but
then fails to jump to it and to execute a program given as an argument
to openvt. It seems like the child process created by the openvt
program crashes just after the call to the fork() system call.
I have the following line in the `/etc/inittab' to deal with the
keyboard requests:
............
kb::kbrequest:/bin/openvt -s /bin/login
............
I've tried to use the `-w' and `-e' options in addition to the `-s'
one in the line above, which works well enough. However in this
case init would wait the completion of the started program and
so it would prevent a user from opening another virtual terminal by
pressing <left-Alt>+<UpArrow> again. That would really restrict the
functionality of the program. Moreover, because of the permission
settings in my system, an ordinary user may not open a tty device by
running openvt from the shell command prompt, and so this limits the
number of virtual terminals that can be used by an ordinary user
by a maximum of two (the first is opened by mingetty after the
system is loaded).
The program works as it is to do if I run it from the shell command
prompt as the root user.
I've tried the openvt program from the older kbd-0.99 package. That
works well in either case: whether it is started from the command
prompt of the shell by hand or from `/etc/inittab' by init on a keyboard
request.
The Possible Reason.
In fact, there is one essential difference between running a program from
the shell by the root user and running it from the `/etc/inittab' by the
init process.
If one runs a program from the command prompt of a shell, and the program
doesn't create its own session somehow (e.g. by doing
setpgid(0, getpgid(getppid())) and setsid()), it always belongs to the
same session as the shell does. In that case, if the program opens a
terminal device, leaves it open, spawns children, and then exits
immediately, that can do no harm to the children. They will continue
running until they exit by themselves, or a signal kills them.
On the other hand, when the same program is started by the init process
like with the line from my `/etc/inittab' above, the new process inevitably
becomes the leader of a new session without a controlling terminal. If such
a process opens a terminal device which is not the controlling terminal
of another session, the device turns the controlling terminal of this
session and the process becomes its controlling process. The group of the
controlling process in turn becomes the foreground group. If the process
leaves the device open and then spawns children, they inherit its
controlling terminal as well as its group. Now, when the controlling process
exits, a SIGHUP signal is delivered to all the processes in the foreground
group, including the children, killing them by default (see libc info,
``Termination Internals'').
To see how this affects the openvt behavior, look at the listing below.
......................................
038
039 int
040 main(int argc, char *argv[])
041 {
......................................
128 if (ioctl(consfd, VT_GETSTATE, &vtstat) < 0) {
......................................
130 return(4);
131 }
132
133 if (vtno == -1) {
134 if ((ioctl(consfd, VT_OPENQRY, &vtno) < 0) || (vtno == -1)) {
......................................
137 return(3);
138 }
139 } else if (!force) {
......................................
151 }
152
153 sprintf(vtname, VTNAME, vtno);
154
155 /* Can we open the vt we want? */
156 if ((fd0 = open(vtname, O_RDWR)) == -1) {
157 int errsv = errno;
158 if (!optc) {
159 /* We found vtno ourselves - it is free according
160 to the kernel, but we cannot open it. Maybe X
161 used it and did a chown. Try a few vt's more
162 before giving up. Note: the 16 is a kernel limitation. */
163 for (i=vtno+1; i<16; i++) {
164 if((vtstat.v_state & (1<<i)) == 0) {
165 sprintf(vtname, VTNAME, i);
166 if ((fd0 = open(vtname, O_RDWR)) >= 0) {
167 vtno = i;
168 goto got_vtno;
169 }
170 }
171 }
172 sprintf(vtname, VTNAME, vtno);
173 }
174 fprintf(stderr, _("openvt: Unable to open %s: %s\n"),
175 vtname, strerror(errsv));
176 return(5);
177 }
178 got_vtno:
179
......................................
229
230 if (direct_exec || ((pid = fork()) == 0)) {
231 /* leave current vt */
232 if (!direct_exec) {
......................................
240 if (setsid() < 0) {
......................................
245 }
246 }
250 if ((fd1 = open(vtname, O_RDWR)) == -1) { /* strange ... */
......................................
256 }
......................................
277 close(fd0);
......................................
285 if (ioctl(fd1, TIOCSCTTY, (char *)1)) {
......................................
288 _exit (1);
289 }
......................................
293 if(as_user)
294 execlp("login", "login", "-f", username, NULL);
295 else if (def_cmd)
296 execlp(cmd, def_cmd, NULL);
297 else
298 execvp(cmd, &argv[optind]);
......................................
301 }
302
303 if ( pid < 0 ) {
304 perror("openvt: fork() error");
305 return(6);
306 }
307
308 if ( do_wait ) {
309 wait(NULL);
......................................
326 }
327
328 return 0;
329 }
Fig. 1. This listing shows the part of the openvt program where
a race condition takes place.
Here the parent process first looks for the available virtual terminals
at lines #128--#151, and then opens the first available one with the
appropriate read and write permissions at lines #155-#178. It is clear
that the `open()' call is only needed to choose a terminal suitable for
reading and writing among all available virtual terminals, for the
parent openvt process will never use it and the child openvt process
will open it anew at line #250. However, the parent openvt never closes
the terminal device file until the final return from the main function,
and here lies the problem.
If the program was started by init, the parent openvt process is a
session leader, and so the terminal becomes the controlling terminal
of the session. As the consequence, the parent openvt process becomes
the controlling process of the session. Since it keeps the device
file open, it remains the controlling process till the exit.
Now, the parent openvt process does fork() system call at line #230,
evaluates two very simple expressions in `if-else' statements
(lines #303, #308), and then exits immediately at line #328 if the `-w'
option has not been given (i.e. if do_wait == FALSE). At this point, if
the process was started by init, the SIGHUP signal is sent to the child
if it is still in the session. The child openvt process, at the same
time, tries to do some job while in fork(), and do the call to setsid()
at line #240 to create its own session and so get out of its parent's
one. That takes the child some while, during which it can catch the
SIGHUP signal.
Thus, in the case of the openvt program, all that I've written above means
that there appears a race condition at lines #230 to #328 when openvt is
started by init. If the parent openvt process terminates before
the child's call to the setsid() completes (line #240), the child openvt
process will receive the SIGHUP and die before it itself becomes the
leader of another session and so gets protected form that signal. On the
other hand, if the child is fast enough to accomplish the setsid() call
before the parent's exit, it will escape the signal and survive.
The former scenario seems to always take place with the recent version
of openvt on my system (CPU -- Pentium 4, 2.4 GHz; RAM -- 512 MByte;
OS -- Linux-2.6.37; libc -- eglibc-2.11.2). The latter may turn out to
be much more probable on others, as I could see from the source.
Note that when openvt is started from the shell, the parent openvt is not
a session leader and so the termination of the parent process does not
imply delivering the SIGHUP signal to its child, and so does not affect
it in either case.
My experiments with the sources (see ``Other Possible Solutions'' below)
have shown that this is the only possible reason for the problem.
The Proposed Solution.
I should propose several simple possible solutions to the problem. All
are portable enough. While I prefer the first one (I've sent the corrected
`openvt.c' file as an attachment), I've tried all of them, and they all
do well.
The simplest solution that I can think of is to close the controlling
terminal before the fork() system call at line #230. It may be the best
to simply move the `close(fd0);' statement from line #277 to line #179,
i.e. to place it where it used to be in the earlier versions of the
openvt program, right after the label `got_vtno:'. Having closed the
controlling terminal before forking the child, the parent relinquishes
the controlling terminal, and so it won't be the controlling process since
then. Because of that, the SIGHUP signal will never be sent to the child
when the parent exits.
It works fine on my system. Though, according to the ``UnixWare 7
Documentation'' available at
`http://uw714doc.sco.com/en/SDK_sysprog/_The_Controlling-Terminal_and_Pr.html',
it would be safer yet to open the terminal device file with the `O_NOCTTY'
flag at lines #156, #166, which would prevent making it the controlling
terminal of the parent process. In this case, it is even possible not
to move the `close(fd0);' statement as suggested above. However, the
only reasonable purpose of opening the terminal here is to check if it
can be opened for reading and writing. Thus, there is no sense in keeping
the device open and so it had better be closed right after the check, I
think.
Actually, there might be yet another potential timing problem if two or
more openvt instances were started at the same time (say from a shell
script), all found the same virtual terminal to be free at lines #128--#151
and then opened it. In that case, only the last process which does the
`ioctl(fd1, TIOCSCTTY, (char *)1)' system call at line #285 will finally
make it the controlling terminal (see tty_ioctl(4)). All the others in turn
will lose their controlling terminal and so won't have the controlling
terminal at all, but will still be running and going on using it. This
must really lead to a good deal of mess on that terminal.
Although I've never seen that in practice, it is still possible. Moreover,
as an experiment, I have successfully simulated such a behavior by
introducing a delay `sleep(10);' before the `if-else' statement at lines
#156--#177 in the original `openvt.c' from kbd-1.15.2, and starting two
instances of the program one after another from the shell as background
jobs.
It's clear form that experiment that the less the time interval between
finding a terminal to be free and opening it, the less the probability that
several openvt instances open the same terminal. In fact, the proposed
movement of the `close(fd0);' statement may even exacerbate the problem,
increasing that interval. And yet the latter doesn't matter much. Because
it is impossible to accomplish the testing and opening a terminal in one
indivisible operation, in any case, always there is a possibility that
several openvt processes will use the same virtual terminal. So it would be
better in that case if all but one processes closed the terminal device file
and exited. And that can be easily managed, with the change suggested above
having been made.
Note that the only reason for the ioctl system call at line #285 in the
recent version of the openvt program was to guarantee that the child
process, which is a session leader, always gets the controlling terminal
from its parent. That is indeed necessary if the program has been started
by init and the parent process, which is also a session leader in this case,
may still be running and holding the controlling terminal. However, if the
parent opens the terminal device file with the `O_NOCTTY' flag and closes
it before forking the child as proposed above, this ioctl call is never
needed. Note also that according to the standards, when a session leader
without a controlling terminal opens a terminal device file and the flag
O_NOCTTY is clear on open, that terminal becomes the controlling terminal
assigned to the session leader if and only if the terminal is not already
assigned to some session.
So if we remove the unnecessary ioctl system call at line #285, an openvt
child's call to `fd1 = open(vtname, O_RDWR)' at line #250 is now enough
to ensure that only the first child openvt process that opens the
terminal makes it the controlling terminal for its session, with all the
others failing to do so. Thus, all that remains for the program to do is
to test then with `tcgetsid(fd1)' (see tcgetsid(3) for details) whether
the device file given by the file descriptor fd1 has really become the
controlling terminal for its session. If the call to the function fails,
returning `-1', the process should give up the terminal and exit;
otherwise, it may continue running. I should say that the simplest way to
introduce the test is to change the statement
285 if (ioctl(fd1, TIOCSCTTY, (char *)1)) {
286 perror("ioctl TIOCSCTTY");
287 fflush(stderr);
288 _exit (1);
289 }
for
285 if(tcgetsid(fd1) == -1) {
286 perror("tcgetsid");
287 fflush(stderr);
288 _exit (1);
289 }
That is precisely those changes to `openvt.c' which I have made on my
system. If that solution is not appropriate for some reason, here are
others. I am not sure that these are so good: I just used them when I
was tracking down the cause of the problem. However, they illustrate
the approach, and I think they may still be of some use and so worth
mentioning.
Other Possible Solutions.
In the listings below, lines added by me always have the same number as
the previous line does in the original `openvt.c'. To refer to such lines
and distinguish them from one another in the following text, the equal
line numbers are indexed with letters in alphabetical order, e.g 307a,
307b, etc.
A second solution would be to put a call to the `sleep()' function just
before the final `return 0;' from the `main()' function (see Fig. 2).
Such a delay would postpone the delivery of the SIGHUP signal on the
parent's exit for a while and thus would give the child some time to
accomplish the setsid() call.
......................................
307
307a if( getpid() == getsid(0) ) {
307b sleep(1);
307c return 0;
307d };
307e
308 if ( do_wait ) {
309 wait(NULL);
......................................
326 }
327
328 return 0;
329 }
Fig. 2 A possible solution which exploits a delay before exiting
the parent process. Lines #307a--307e implementing the delay
have been added to the original `openvt.c' from kbd-1.15.2.
The conditional statement `if( getpid() == getsid(0) ) { ... };'
in lines #307a--#307e ensures that the `sleep(1)' delay is applied
only if the parent process is started by init and so it is the
leader of a session. A delay of one second is more than enough for
the child to call to setsid() and to eventually become the leader
of its own session. Note that init will not be able to open a new
virtual terminal on a user request for the delay period (1s.) in
this case. It will wait until the parent openvt process terminates.
Note also that the parent process waiting for the child termination
due to the `-w' option erroneously given in `/etc/inittab' makes
no sense here (lines from #308 to #327) and so it is bypassed with
the `return 0;' statement at line #307c.
Another, somewhat similar solution is to have the parent wait until
the child let the parent know somehow of its having accomplished
the setsid() call (see Fig. 3). The following solution utilizes a
possible sending the SIGTERM signal by the child at line #245a, and
the waiting for a signal by the parent; the latter, in fact, has
already been implemented in the original `openvt.c' at lines
#308--#326 (see wait(2)).
......................................
038
039 int
040 main(int argc, char *argv[])
041 {
042
042a int init_flag; /* is TRUE iff the process's been started
by `init' */
042b pid_t PPID; /* The parent process's PID
......................................
229
229a PPID = getpid();
229b init_flag = ( PPID == getsid(0) );
229c do_wait = do_wait || init_flag;
229d
230 if (direct_exec || ((pid = fork()) == 0)) {
231 /* leave current vt */
232 if (!direct_exec) {
......................................
240 if (setsid() < 0) {
......................................
245 }
245a if(init_flag) kill(PPID, SIGTERM);
246 }
......................................
301 }
302
......................................
308 if ( do_wait ) {
309 wait(NULL);
......................................
326 }
327
328 return 0;
329 }
Fig. 3 Another solution which exploits a delay before exiting
the parent process. Here the parent waits until the child
process sends it a SIGTERM signal. Lines #042a, #042b,
#229a--#229d, #245a have been added to the original
`openvt.c' from kbd-1.15.2.
A fourth solution is to protect somehow the child `openvt' process
from the SIGHUP signal that may be delivered to it on the parent's
exit. I should say that the most portable way to do so is to install
SIG_IGN signal handler just before the call to the fork() at line #230
(see Fig. 4). In that case the child process inherits the signal
handler from its parent (see libc info ``Creating a Process''), and
so it gets protected from the parent's death. The following call to
a function of the exec family at lines #293--298 would preserve the
SIG_IGN handler (see libc info, ``Executing a File'') for the SIGHUP
signal, so the default one (SIG_DFL) has to be restored before the
call.
......................................
038
039 int
040 main(int argc, char *argv[])
041 {
042
042a int init_flag; /* is TRUE iff the process's been started
by `init' */
......................................
229
229a init_flag = ( getpid() == getsid(0) ); /* A critical section!!! */
229b if( !direct_exec && init_flag ) { /* Protect the child from the
`SIGHUP' */
229c signal(SIGHUP, SIG_IGN); /* delivered on the parent's
exit */
229d do_wait = FALSE;
229e };
229f
230 if (direct_exec || ((pid = fork()) == 0)) {
231 /* leave current vt */
232 if (!direct_exec) {
......................................
240 if (setsid() < 0) {
......................................
245 }
245a /* End of the critical section */
245b if(init_flag) signal(SIGHUP, SIG_DFL); /* Restore the
default handler */
246 }
......................................
301 }
302
......................................
308 if ( do_wait ) {
309 wait(NULL);
......................................
326 }
327
328 return 0;
329 }
Fig. 4. A possible solution where the SIG_IGN signal handler is
installed (lines #229a--#229e) to protect the child
process from the SIGHUP signal sent on the parent's exit.
Lines #042a, #229a--#229f, #245a and #245b have been added
to the original `openvt.c' from kbd-1.15.2.
Once again, the statements `init_flag = ( getpid() == getsid(0) );
if( !direct_exec && init_flag ) { ... };' at lines #229a--#229f ensure
that the SIG_IGN signal handler is installed if and only if the parent
process is started by init. After the call to setsid() at line #240,
the child becomes immune from the SIGHUP signal, and the default handler
is restored at line #245b. The parent process waiting for the child
completion in lines #308--326, if the `-w' option should have been given,
makes no sense in this case, and so it is bypassed by setting the
`do_wait' variable to FALSE at line #229c.
P.S.
There is yet another bug at lines #206, #208 (see the listing below).
Here the memory chunk requested by a call to `malloc()' for the
string `cmd' is three or four bytes shorter than the real number of
symbols eventually put into it at lines #211--214 and #216--219. Thus,
at line #206, the statement `cmd = malloc(strlen(def_cmd + 2));'
should be replaced with `cmd = malloc(strlen(def_cmd) + 2);'. Similarly,
at line #208, the statement `cmd = malloc(strlen(argv[optind] + 2));'
should be replaced by `cmd = malloc(strlen(argv[optind]) + 2);'.
202 if (!(argc > optind)) {
203 def_cmd = getenv("SHELL");
204 if (def_cmd == NULL)
205 usage(0);
206 cmd = malloc(strlen(def_cmd + 2));
207 } else {
208 cmd = malloc(strlen(argv[optind] + 2));
209 }
210
211 if (login)
212 strcpy(cmd, "-");
213 else
214 cmd[0] = '\0';
215
216 if (def_cmd)
217 strcat(cmd, def_cmd);
218 else
219 strcat(cmd, argv[optind]);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openvt.c
Type: text/x-csrc
Size: 10775 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-kbd-devel/attachments/20110415/d00b84dc/attachment-0001.c>
More information about the Pkg-kbd-devel
mailing list