Subject: Re: SE GC83 GPRS card: pppd problem
To: Christos Zoulas <christos@zoulas.com>
From: Jachym Holecek <freza@liberouter.org>
List: current-users
Date: 01/30/2006 18:00:20
> On Jan 30,  5:16pm, freza@liberouter.org (Jachym Holecek) wrote:
> -- Subject: Re: SE GC83 GPRS card: pppd problem
> | 
> | I did the same last night (but offline) -- the problem was in main.c
> | (in case you really mean sys-bsd.c, I can take a look later in the
> | evening)
> 
> I really meant sys-bsd.c ...

dist/pppd/pppd/main.c contains unhandled corner case, the patch
below fixed the problem I've reproduced with Alex's setup. The
second occupy_stdfds() call is key.

	-- Jachym

--- main.c	2006-01-30 17:52:56.000000000 +0100
+++ /home/jh/main.c	2006-01-30 17:54:17.000000000 +0100
@@ -1525,6 +1525,23 @@
 }
 
 /*
+ * occupy_stdfds - make sure fds 0, 1, 2 are occupied by pointing them
+ * to the /dev/null device.
+ */
+static void
+occupy_stdfds(void)
+{
+	int fd;
+
+	while ((fd = dup(fd_devnull)) >= 0) {
+		if (fd > 2) {
+			close(fd);
+			break;
+		}
+	}
+}
+
+/*
  * safe_fork - Create a child process.  The child closes all the
  * file descriptors that we don't want to leak to a script.
  * The parent waits for the child to do this before returning.
@@ -1535,16 +1552,11 @@
 safe_fork(int infd, int outfd, int errfd)
 {
 	pid_t pid;
-	int fd, pipefd[2];
+	int pipefd[2];
 	char buf[1];
 
-	/* make sure fds 0, 1, 2 are occupied (probably not necessary) */
-	while ((fd = dup(fd_devnull)) >= 0) {
-		if (fd > 2) {
-			close(fd);
-			break;
-		}
-	}
+	/* make sure the pipes will be above fd 2 */
+	occupy_stdfds();
 
 	if (pipe(pipefd) == -1)
 		pipefd[0] = pipefd[1] = -1;
@@ -1568,6 +1580,9 @@
 	tdb_close(pppdb);
 #endif
 
+	/* we've closed some descriptors, need to ensure again */
+	occupy_stdfds();
+
 	/* make sure infd, outfd and errfd won't get tromped on below */
 	if (infd == 1 || infd == 2)
 		infd = dup(infd);
@@ -1577,28 +1592,31 @@
 		errfd = dup(errfd);
 
 	/* dup the in, out, err fds to 0, 1, 2 */
-	if (infd != 0)
+	if (infd != 0) {
 		dup2(infd, 0);
-	if (outfd != 1)
+		close(infd);
+	}
+	if (outfd != 1) {
 		dup2(outfd, 1);
-	if (errfd != 2)
+		close(outfd);
+	}
+	if (errfd != 2) {
 		dup2(errfd, 2);
+		close(errfd);
+	}
 
 	closelog();
 	if (log_to_fd > 2)
 		close(log_to_fd);
-	if (the_channel->close)
+	if (the_channel->close) {
 		(*the_channel->close)();
-	else
-		close(devfd);	/* some plugins don't have a close function */
+		notice("teh_channel close");
+	} else {
+		if (devfd > 0)
+			close(devfd);	/* some plugins don't have a close() */
+	}
 	close(fd_ppp);
 	close(fd_devnull);
-	if (infd != 0)
-		close(infd);
-	if (outfd != 1)
-		close(outfd);
-	if (errfd != 2)
-		close(errfd);
 
 	notify(fork_notifier, 0);
 	close(pipefd[0]);
@@ -1631,6 +1649,8 @@
     ++conn_running;
     pid = safe_fork(in, out, errfd);
 
+    if (pid == 0)
+	if (log_to_fd
     if (pid != 0 && log_to_fd < 0)
 	close(errfd);