Random minor fixes for movemail

* movemail.c: Include <stdbool.h> and <signal.h>.
(waitpid) [WINDOWSNT]: New macro.
(wait) [WINDOWSNT]: Remove.
(main, popmail, pop_retr, mbx_write, mbx_delimit_begin)
(mbx_delimit_end): Use bool for boolean.
(main): Simplify #if usage a bit.
(main): Don't assume EOF == -1.  Prefer 'return' to 'exit'.  Don't
possibly unlink lockname twice, as that's a race condition.  Set
SIGCHLD to SIG_DFL to work around SysV misfeature.  Check for fork
failure.  Use waitpid, not wait, to avoid a race condition in the
unlikely case where we start up with a child.
(NOTOK, OK): Remove, in favor of plain boolean.
(popmail, pop_retr): Don't get confused about errno, e.g., ferror
need not set errno.
(popmail): Use fclose (mbf), not close (fileno (mbf)), to also
detect any stream-related errors (e.g., memory exhaustion).
(pop_retr): Report pop errors separately, since caller now does
errno reporting.
(mbx_write, mbx_delimit_begin, mbx_delimit_end): Check < 0, not ==
EOF, as it's a bit faster and (in theory) pickier.

2 files changed, 117 insertions, 104 deletions

diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index 83855afa675..97868094bda 100644
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,27 @@
+2015-03-06  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Random minor fixes for movemail
+	* movemail.c: Include <stdbool.h> and <signal.h>.
+	(waitpid) [WINDOWSNT]: New macro.
+	(wait) [WINDOWSNT]: Remove.
+	(main, popmail, pop_retr, mbx_write, mbx_delimit_begin)
+	(mbx_delimit_end): Use bool for boolean.
+	(main): Simplify #if usage a bit.
+	(main): Don't assume EOF == -1.  Prefer 'return' to 'exit'.  Don't
+	possibly unlink lockname twice, as that's a race condition.  Set
+	SIGCHLD to SIG_DFL to work around SysV misfeature.  Check for fork
+	failure.  Use waitpid, not wait, to avoid a race condition in the
+	unlikely case where we start up with a child.
+	(NOTOK, OK): Remove, in favor of plain boolean.
+	(popmail, pop_retr): Don't get confused about errno, e.g., ferror
+	need not set errno.
+	(popmail): Use fclose (mbf), not close (fileno (mbf)), to also
+	detect any stream-related errors (e.g., memory exhaustion).
+	(pop_retr): Report pop errors separately, since caller now does
+	errno reporting.
+	(mbx_write, mbx_delimit_begin, mbx_delimit_end): Check < 0, not ==
+	EOF, as it's a bit faster and (in theory) pickier.
+
 2015-02-27  Mark Laws  <mdl@60hz.org>
 
 	Support daemon mode on MS-Windows (bug#19688)
diff --git a/lib-src/movemail.c b/lib-src/movemail.c
index 5008c9b15f7..1618a6980e9 100644
--- a/lib-src/movemail.c
+++ b/lib-src/movemail.c
@@ -59,6 +59,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/file.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <errno.h>
 #include <time.h>
@@ -66,6 +67,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <getopt.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <signal.h>
 #include <string.h>
 #include "syswait.h"
 #ifdef MAIL_USE_POP
@@ -81,7 +83,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #undef access
 #undef unlink
 #define fork() 0
-#define wait(var) (*(var) = 0)
+#define waitpid(child, var, flags) (*(var) = 0)
 /* Unfortunately, Samba doesn't seem to properly lock Unix files even
    though the locking call succeeds (and indeed blocks local access from
    other NT programs).  If you have direct file access using an NFS
@@ -134,11 +136,11 @@ static void error (const char *s1, const char *s2, const char *s3);
 static _Noreturn void pfatal_with_name (char *name);
 static _Noreturn void pfatal_and_delete (char *name);
 #ifdef MAIL_USE_POP
-static int popmail (char *mailbox, char *outfile, int preserve, char *password, int reverse_order);
-static int pop_retr (popserver server, int msgno, FILE *arg);
-static int mbx_write (char *line, int len, FILE *mbf);
-static int mbx_delimit_begin (FILE *mbf);
-static int mbx_delimit_end (FILE *mbf);
+static int popmail (char *, char *, bool, char *, bool);
+static bool pop_retr (popserver, int, FILE *);
+static bool mbx_write (char *, int, FILE *);
+static bool mbx_delimit_begin (FILE *);
+static bool mbx_delimit_end (FILE *);
 #endif
 
 #if (defined MAIL_USE_MAILLOCK						\
@@ -166,23 +168,21 @@ main (int argc, char **argv)
   int indesc, outdesc;
   ssize_t nread;
   int wait_status;
-  int c, preserve_mail = 0;
+  int c;
+  bool preserve_mail = false;
 
 #ifndef MAIL_USE_SYSTEM_LOCK
   struct stat st;
   int tem;
-  char *lockname;
   char *tempname;
   size_t inname_len, inname_dirlen;
   int desc;
 #endif /* not MAIL_USE_SYSTEM_LOCK */
 
-#ifdef MAIL_USE_MAILLOCK
-  char *spool_name;
-#endif
+  char *spool_name = 0;
 
 #ifdef MAIL_USE_POP
-  int pop_reverse_order = 0;
+  bool pop_reverse_order = false;
 # define ARGSTR "pr"
 #else /* ! MAIL_USE_POP */
 # define ARGSTR "p"
@@ -193,19 +193,19 @@ main (int argc, char **argv)
 
   delete_lockname = 0;
 
-  while ((c = getopt (argc, argv, ARGSTR)) != EOF)
+  while (0 <= (c = getopt (argc, argv, ARGSTR)))
     {
       switch (c) {
 #ifdef MAIL_USE_POP
       case 'r':
-	pop_reverse_order = 1;
+	pop_reverse_order = true;
 	break;
 #endif
       case 'p':
-	preserve_mail++;
+	preserve_mail = true;
 	break;
       default:
-	exit (EXIT_FAILURE);
+	return EXIT_FAILURE;
       }
     }
 
@@ -223,7 +223,7 @@ main (int argc, char **argv)
 #else
       fprintf (stderr, "Usage: movemail [-p] inbox destfile%s\n", "");
 #endif
-      exit (EXIT_FAILURE);
+      return EXIT_FAILURE;
     }
 
   inname = argv[optind];
@@ -244,7 +244,7 @@ main (int argc, char **argv)
       status = popmail (inname + 3, outname, preserve_mail,
 			(argc - optind == 3) ? argv[optind+2] : NULL,
 			pop_reverse_order);
-      exit (status);
+      return status;
     }
 
   if (setuid (getuid ()) < 0)
@@ -253,18 +253,15 @@ main (int argc, char **argv)
 #endif /* MAIL_USE_POP */
 
 #ifndef DISABLE_DIRECT_ACCESS
+
+  char *lockname = 0;
+
 #ifndef MAIL_USE_MMDF
 #ifndef MAIL_USE_SYSTEM_LOCK
 #ifdef MAIL_USE_MAILLOCK
   spool_name = mail_spool_name (inname);
-  if (spool_name)
-    {
-#ifdef lint
-      lockname = 0;
-#endif
-    }
-  else
 #endif
+  if (! spool_name)
     {
       /* Use a lock file named after our first argument with .lock appended:
 	 If it exists, the mail file is locked.  */
@@ -292,7 +289,7 @@ main (int argc, char **argv)
 	continue;
       tempname = xmalloc (inname_dirlen + sizeof "EXXXXXX");
 
-      while (1)
+      while (true)
 	{
 	  /* Create the lock file, but not under the lock file name.  */
 	  /* Give up if cannot do that.  */
@@ -328,7 +325,10 @@ main (int argc, char **argv)
 	    {
 	      time_t now = time (0);
 	      if (st.st_ctime < now - 300)
-		unlink (lockname);
+		{
+		  unlink (lockname);
+		  lockname = 0;
+		}
 	    }
 	}
 
@@ -337,15 +337,20 @@ main (int argc, char **argv)
 #endif /* not MAIL_USE_SYSTEM_LOCK */
 #endif /* not MAIL_USE_MMDF */
 
-  if (fork () == 0)
+#ifdef SIGCHLD
+  signal (SIGCHLD, SIG_DFL);
+#endif
+
+  pid_t child = fork ();
+  if (child < 0)
+    fatal ("Error in fork; %s", strerror (errno), 0);
+
+  if (child == 0)
     {
       int lockcount = 0;
       int status = 0;
 #if defined (MAIL_USE_MAILLOCK) && defined (HAVE_TOUCHLOCK)
-      time_t touched_lock;
-# ifdef lint
-      touched_lock = 0;
-# endif
+      time_t touched_lock IF_LINT (= 0);
 #endif
 
       if (setuid (getuid ()) < 0 || setregid (-1, real_gid) < 0)
@@ -382,9 +387,9 @@ main (int argc, char **argv)
 #ifdef MAIL_USE_MAILLOCK
       if (spool_name)
 	{
-	  /* The "0 - " is to make it a negative number if maillock returns
+	  /* The "-" is to make it a negative number if maillock returns
 	     non-zero. */
-	  status = 0 - maillock (spool_name, 1);
+	  status = - maillock (spool_name, 1);
 #ifdef HAVE_TOUCHLOCK
 	  touched_lock = time (0);
 #endif
@@ -422,7 +427,7 @@ main (int argc, char **argv)
       {
 	char buf[1024];
 
-	while (1)
+	while (true)
 	  {
 	    nread = read (indesc, buf, sizeof buf);
 	    if (nread < 0)
@@ -464,7 +469,7 @@ main (int argc, char **argv)
 #ifdef MAIL_USE_SYSTEM_LOCK
       if (! preserve_mail)
 	{
-	  if (ftruncate (indesc, 0L) != 0)
+	  if (ftruncate (indesc, 0) != 0)
 	    pfatal_with_name (inname);
 	}
 #endif /* MAIL_USE_SYSTEM_LOCK */
@@ -499,21 +504,18 @@ main (int argc, char **argv)
       if (spool_name)
 	mailunlock ();
 #endif
-      exit (EXIT_SUCCESS);
+      return EXIT_SUCCESS;
     }
 
-  wait (&wait_status);
+  if (waitpid (child, &wait_status, 0) < 0)
+    fatal ("Error in waitpid; %s", strerror (errno), 0);
   if (!WIFEXITED (wait_status))
-    exit (EXIT_FAILURE);
+    return EXIT_FAILURE;
   else if (WEXITSTATUS (wait_status) != 0)
-    exit (WEXITSTATUS (wait_status));
+    return WEXITSTATUS (wait_status);
 
-#if !defined (MAIL_USE_MMDF) && !defined (MAIL_USE_SYSTEM_LOCK)
-#ifdef MAIL_USE_MAILLOCK
-  if (! spool_name)
-#endif /* MAIL_USE_MAILLOCK */
+  if (lockname)
     unlink (lockname);
-#endif /* not MAIL_USE_MMDF and not MAIL_USE_SYSTEM_LOCK */
 
 #endif /* ! DISABLE_DIRECT_ACCESS */
 
@@ -616,12 +618,6 @@ pfatal_and_delete (char *name)
 #include <pwd.h>
 #include <string.h>
 
-#define NOTOK (-1)
-#define OK 0
-
-static char Errmsg[200];	/* POP errors, at least, can exceed
-				   the original length of 80.  */
-
 /*
  * The full valid syntax for a POP mailbox specification for movemail
  * is "po:username:hostname".  The ":hostname" is optional; if it is
@@ -637,10 +633,11 @@ static char Errmsg[200];	/* POP errors, at least, can exceed
  */
 
 static int
-popmail (char *mailbox, char *outfile, int preserve, char *password, int reverse_order)
+popmail (char *mailbox, char *outfile, bool preserve, char *password,
+	 bool reverse_order)
 {
   int nmsgs, nbytes;
-  register int i;
+  int i;
   int mbfi;
   FILE *mbf;
   popserver server;
@@ -690,7 +687,8 @@ popmail (char *mailbox, char *outfile, int preserve, char *password, int reverse
 	}
     }
 
-  if ((mbf = fdopen (mbfi, "wb")) == NULL)
+  mbf = fdopen (mbfi, "wb");
+  if (!mbf)
     {
       pop_close (server);
       error ("Error in fdopen: %s", strerror (errno), 0);
@@ -713,35 +711,28 @@ popmail (char *mailbox, char *outfile, int preserve, char *password, int reverse
     }
 
   for (i = start; i * increment <= end * increment; i += increment)
-    {
-      if (mbx_delimit_begin (mbf) != OK
-	  || pop_retr (server, i, mbf) != OK)
-	{
-	  error ("%s", Errmsg, 0);
-	  close (mbfi);
-	  return EXIT_FAILURE;
-	}
-      mbx_delimit_end (mbf);
-      fflush (mbf);
-      if (ferror (mbf))
-	{
-	  error ("Error in fflush: %s", strerror (errno), 0);
-	  pop_close (server);
-	  close (mbfi);
-	  return EXIT_FAILURE;
-	}
-    }
+    if (! (mbx_delimit_begin (mbf)
+	   && pop_retr (server, i, mbf)
+	   && mbx_delimit_end (mbf)
+	   && fflush (mbf) == 0))
+      {
+	if (errno)
+	  error ("Error in POP retrieving: %s", strerror (errno), 0);
+	pop_close (server);
+	fclose (mbf);
+	return EXIT_FAILURE;
+      }
 
   if (fsync (mbfi) != 0 && errno != EINVAL)
     {
       error ("Error in fsync: %s", strerror (errno), 0);
-      close (mbfi);
+      fclose (mbf);
       return EXIT_FAILURE;
     }
 
-  if (close (mbfi) != 0)
+  if (fclose (mbf) != 0)
     {
-      error ("Error in close: %s", strerror (errno), 0);
+      error ("Error in fclose: %s", strerror (errno), 0);
       return EXIT_FAILURE;
     }
 
@@ -765,7 +756,7 @@ popmail (char *mailbox, char *outfile, int preserve, char *password, int reverse
   return EXIT_SUCCESS;
 }
 
-static int
+static bool
 pop_retr (popserver server, int msgno, FILE *arg)
 {
   char *line;
@@ -773,8 +764,9 @@ pop_retr (popserver server, int msgno, FILE *arg)
 
   if (pop_retrieve_first (server, msgno, &line))
     {
-      snprintf (Errmsg, sizeof Errmsg, "Error from POP server: %s", pop_error);
-      return (NOTOK);
+      error ("Error from POP server: %s", pop_error, 0);
+      errno = 0;
+      return false;
     }
 
   while ((ret = pop_retrieve_next (server, &line)) >= 0)
@@ -782,24 +774,26 @@ pop_retr (popserver server, int msgno, FILE *arg)
       if (! line)
 	break;
 
-      if (mbx_write (line, ret, arg) != OK)
+      if (! mbx_write (line, ret, arg))
 	{
-	  strcpy (Errmsg, strerror (errno));
+	  int write_errno = errno;
 	  pop_close (server);
-	  return (NOTOK);
+	  errno = write_errno;
+	  return false;
 	}
     }
 
   if (ret)
     {
-      snprintf (Errmsg, sizeof Errmsg, "Error from POP server: %s", pop_error);
-      return (NOTOK);
+      error ("Error from POP server: %s", pop_error, 0);
+      errno = 0;
+      return false;
     }
 
-  return (OK);
+  return true;
 }
 
-static int
+static bool
 mbx_write (char *line, int len, FILE *mbf)
 {
 #ifdef MOVEMAIL_QUOTE_POP_FROM_LINES
@@ -811,47 +805,42 @@ mbx_write (char *line, int len, FILE *mbf)
 			    && (a[4] == ' '))
   if (IS_FROM_LINE (line))
     {
-      if (fputc ('>', mbf) == EOF)
-	return (NOTOK);
+      if (fputc ('>', mbf) < 0)
+	return false;
     }
 #endif
   if (line[0] == '\037')
     {
-      if (fputs ("^_", mbf) == EOF)
-	return (NOTOK);
+      if (fputs ("^_", mbf) < 0)
+	return false;
       line++;
       len--;
     }
-  if (fwrite (line, 1, len, mbf) != len)
-    return (NOTOK);
-  if (fputc (0x0a, mbf) == EOF)
-    return (NOTOK);
-  return (OK);
+  return fwrite (line, 1, len, mbf) == len && 0 <= fputc ('\n', mbf);
 }
 
-static int
+static bool
 mbx_delimit_begin (FILE *mbf)
 {
   time_t now = time (NULL);
   struct tm *ltime = localtime (&now);
   if (!ltime)
-    return NOTOK;
+    return false;
 
   char fromline[100];
   if (! strftime (fromline, sizeof fromline,
 		  "From movemail %a %b %e %T %Y\n", ltime))
-    return NOTOK;
-  if (fputs (fromline, mbf) == EOF)
-    return (NOTOK);
-  return (OK);
+    {
+      errno = EOVERFLOW;
+      return false;
+    }
+  return 0 <= fputs (fromline, mbf);
 }
 
-static int
+static bool
 mbx_delimit_end (FILE *mbf)
 {
-  if (putc ('\n', mbf) == EOF)
-    return (NOTOK);
-  return (OK);
+  return 0 <= putc ('\n', mbf);
 }
 
 #endif /* MAIL_USE_POP */