'devfd' fd leak in pppd

'devfd' fd leak in pppd

[Michael Tokarev]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

All recent versions of pppd have a filedescriptor leak -
pppd leaks a filedescriptor to the (serial/pty/whatever)
underlying device on each "redial".

The global `devfd' variable that holds this filedescriptor
is initialized to -1 at the start of the main loop, before
each redial.

The filedescriptor is opened in auth.c:link_required(),
calling the_channel->connect() routine.

Later, again in auth.c, in link_terminated() routine,
there's the following code:

    /*
     * Run disconnector script, if requested.
     * XXX we may not be able to do this if the line has hung up!
     */
    if (devfd >= 0 && the_channel->disconnect) {
        the_channel->disconnect();
        devfd = -1;
    }

For a tty device, there's a tty.c:disconnect_tty() used as
the_channel->disconnect; but it does not *close* any files.
For that, there's the_channel->close field.  Which is only
used in safe_fork() routine when running a child process...

Again, connect_tty() is used (from the_channel->connect())
to start new connection cycle, and that routine starts with

        pty_master = -1;
        pty_slave = -1;
        real_ttyfd = -1;

which, together with devfd = -1 assignment at the beginning
of the main loop effectively leaks the filedescriptor...

Should pppd call the_channel->close() routine in
auth.c:link_terminated(), at the same place where it calls
the_channel->disconnect() ?

Or should the_channel->disconnect() (disconnect_tty() etc)
call corresponding close() routine automagically?  (It seems
the former is better; but there are so many different small
functions out there...)

The attached patch goes the first route above, and it fixes
the problem for me.

Thanks.

/mjt

[-- Attachment #2: ppp-2.4.3-devfdleak.patch --]
[-- Type: text/x-patch, Size: 535 bytes --]

--- auth.c.orig	2004-11-12 13:30:51.000000000 +0300
+++ auth.c	2005-12-08 17:50:44.000000000 +0300
@@ -638,8 +638,13 @@ link_terminated(unit)
      * Run disconnector script, if requested.
      * XXX we may not be able to do this if the line has hung up!
      */
-    if (devfd >= 0 && the_channel->disconnect) {
-	the_channel->disconnect();
+    if (devfd >= 0) {
+	if (the_channel->disconnect)
+	    the_channel->disconnect();
+	if (the_channel->close)
+	    the_channel->close();
+	else
+	    close(devfd);
 	devfd = -1;
     }