GNU bug report logs - #10592
movemail.c's error function and certain compiler flags

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 10592 in the body.
You can then email your comments to 10592 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rob Browning <rlb <at> defaultvalue.org>
To: bug-gnu-emacs <at> gnu.org
Cc: 655118-forwarded <at> bugs.debian.org, 655118 <at> bugs.debian.org,
	Moritz Mühlenhoff <jmm <at> inutil.org>
Subject: Re: Bug#655118: Please enabled hardened build flags
Date: Mon, 23 Jan 2012 23:05:26 -0600

(Sorry, accidentally hit send too early.)

Mortiz updated Emacs to support -Wformat -Wformat-security
-Werror=format-security.

Here are the relevant changes (further details are available at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=655118):

diff -aur emacs23-23.3+1.orig/lib-src/movemail.c emacs23-23.3+1/lib-src/movemail.c
--- emacs23-23.3+1.orig/lib-src/movemail.c	2011-12-29 05:07:27.000000000 +0100
+++ emacs23-23.3+1/lib-src/movemail.c	2012-01-08 17:31:22.000000000 +0100
@@ -615,11 +615,11 @@
 {
   fprintf (stderr, "movemail: ");
   if (s3)
-    fprintf (stderr, s1, s2, s3);
+    fprintf (stderr, "%s%s%s", s1, s2, s3);
   else if (s2)
-    fprintf (stderr, s1, s2);
+    fprintf (stderr, "%s%s", s1, s2);
   else
-    fprintf (stderr, s1);
+    fprintf (stderr, "%s", s1);
   fprintf (stderr, "\n");
 }
 
Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

Message #8 received at 10592 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Rob Browning <rlb <at> defaultvalue.org>
Cc: 10592 <at> debbugs.gnu.org, 655118-forwarded <at> bugs.debian.org, jmm <at> inutil.org,
	655118 <at> bugs.debian.org
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Tue, 24 Jan 2012 01:06:40 -0500

> From: Rob Browning <rlb <at> defaultvalue.org>
> Date: Mon, 23 Jan 2012 23:05:26 -0600
> Cc: 655118 <at> bugs.debian.org, 655118-forwarded <at> bugs.debian.org,
> 	Moritz Mühlenhoff <jmm <at> inutil.org>
> 
> --- emacs23-23.3+1.orig/lib-src/movemail.c	2011-12-29 05:07:27.000000000 +0100
> +++ emacs23-23.3+1/lib-src/movemail.c	2012-01-08 17:31:22.000000000 +0100
> @@ -615,11 +615,11 @@
>  {
>    fprintf (stderr, "movemail: ");
>    if (s3)
> -    fprintf (stderr, s1, s2, s3);
> +    fprintf (stderr, "%s%s%s", s1, s2, s3);
>    else if (s2)
> -    fprintf (stderr, s1, s2);
> +    fprintf (stderr, "%s%s", s1, s2);
>    else
> -    fprintf (stderr, s1);
> +    fprintf (stderr, "%s", s1);
>    fprintf (stderr, "\n");
>  }

How can this possibly be TRT?  The commentary to this function says:

  /* Print error message.  `s1' is printf control string, `s2' and `s3'
     are args for it or null. */

If S1 is the printf control string, how will printing it with %s DTRT?
E.g., in this invocation:

      error ("Error connecting to POP server: %s", pop_error, 0);

or in this one:

      error ("Error in open: %s, %s", strerror (errno), outfile);

I think the right fix for this is to declare `error' with the
appropriate printf attribute.  Alternatively, you could use variable
argument lists and call vprintf instead.

Message #11 received at 10592 <at> debbugs.gnu.org (full text, mbox):

From: Rob Browning <rlb <at> defaultvalue.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 10592 <at> debbugs.gnu.org, 655118-forwarded <at> bugs.debian.org, jmm <at> inutil.org,
	655118 <at> bugs.debian.org
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Tue, 24 Jan 2012 10:17:10 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

> How can this possibly be TRT?  The commentary to this function says:

I actually wondered the same thing while I was going to sleep.
Obviously I got in too big a hurry last night.  I'll fix it correctly
myself and re-send.

Thanks, and apologies for the noise.
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

Message #16 received at 10592 <at> debbugs.gnu.org (full text, mbox):

From: Rob Browning <rlb <at> defaultvalue.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 10592 <at> debbugs.gnu.org, 655118-forwarded <at> bugs.debian.org, jmm <at> inutil.org,
	655118 <at> bugs.debian.org
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Tue, 24 Jan 2012 20:22:52 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

> I think the right fix for this is to declare `error' with the
> appropriate printf attribute.  Alternatively, you could use variable
> argument lists and call vprintf instead.

Would something like this be acceptable, and if not, how would you like
to see it adjusted?  The patch changes error() to use an ANSI
declaration, and it relies on the printf format attribute:

diff --git a/lib-src/movemail.c b/lib-src/movemail.c
index 58add49..6b2fc20 100644
--- a/lib-src/movemail.c
+++ b/lib-src/movemail.c
@@ -60,6 +60,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <sys/file.h>
 #include <stdio.h>
 #include <errno.h>
+#include <stdarg.h>
 #include <time.h>
 
 #include <getopt.h>
@@ -152,7 +153,7 @@ extern char *rindex __P((const char *, int));
 #endif
 
 void fatal ();
-void error ();
+void error (const char *template, ...) __attribute__ ((format (printf, 1, 2)));
 void pfatal_with_name ();
 void pfatal_and_delete ();
 char *concat ();
@@ -610,16 +611,13 @@ fatal (s1, s2, s3)
    are args for it or null. */
 
 void
-error (s1, s2, s3)
-     char *s1, *s2, *s3;
+error (const char *template, ...)
 {
+  va_list ap;
   fprintf (stderr, "movemail: ");
-  if (s3)
-    fprintf (stderr, s1, s2, s3);
-  else if (s2)
-    fprintf (stderr, s1, s2);
-  else
-    fprintf (stderr, s1);
+  va_start (ap, template);
+  vfprintf (stderr, template, ap);
+  va_end (ap);
   fprintf (stderr, "\n");
 }
 
@@ -733,13 +731,13 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
   server = pop_open (hostname, user, password, POP_NO_GETPASS);
   if (! server)
     {
-      error ("Error connecting to POP server: %s", pop_error, 0);
+      error ("Error connecting to POP server: %s", pop_error);
       return EXIT_FAILURE;
     }
 
   if (pop_stat (server, &nmsgs, &nbytes))
     {
-      error ("Error getting message count from POP server: %s", pop_error, 0);
+      error ("Error getting message count from POP server: %s", pop_error);
       return EXIT_FAILURE;
     }
 
@@ -761,7 +759,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
   if ((mbf = fdopen (mbfi, "wb")) == NULL)
     {
       pop_close (server);
-      error ("Error in fdopen: %s", strerror (errno), 0);
+      error ("Error in fdopen: %s", strerror (errno));
       close (mbfi);
       unlink (outfile);
       return EXIT_FAILURE;
@@ -785,7 +783,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
       mbx_delimit_begin (mbf);
       if (pop_retr (server, i, mbf) != OK)
 	{
-	  error ("%s", Errmsg, 0);
+	  error ("%s", Errmsg);
 	  close (mbfi);
 	  return EXIT_FAILURE;
 	}
@@ -793,7 +791,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
       fflush (mbf);
       if (ferror (mbf))
 	{
-	  error ("Error in fflush: %s", strerror (errno), 0);
+	  error ("Error in fflush: %s", strerror (errno));
 	  pop_close (server);
 	  close (mbfi);
 	  return EXIT_FAILURE;
@@ -809,14 +807,14 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
 #ifdef BSD_SYSTEM
   if (fsync (mbfi) < 0)
     {
-      error ("Error in fsync: %s", strerror (errno), 0);
+      error ("Error in fsync: %s", strerror (errno));
       return EXIT_FAILURE;
     }
 #endif
 
   if (close (mbfi) == -1)
     {
-      error ("Error in close: %s", strerror (errno), 0);
+      error ("Error in close: %s", strerror (errno));
       return EXIT_FAILURE;
     }
 
@@ -825,7 +823,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
       {
 	if (pop_delete (server, i))
 	  {
-	    error ("Error from POP server: %s", pop_error, 0);
+	    error ("Error from POP server: %s", pop_error);
 	    pop_close (server);
 	    return EXIT_FAILURE;
 	  }
@@ -833,7 +831,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
 
   if (pop_quit (server))
     {
-      error ("Error from POP server: %s", pop_error, 0);
+      error ("Error from POP server: %s", pop_error);
       return EXIT_FAILURE;
     }
 
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

Message #19 received at 10592 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Rob Browning <rlb <at> defaultvalue.org>
Cc: 10592 <at> debbugs.gnu.org, 655118-forwarded <at> bugs.debian.org, jmm <at> inutil.org,
	655118 <at> bugs.debian.org
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Wed, 25 Jan 2012 01:40:26 -0500

> From: Rob Browning <rlb <at> defaultvalue.org>
> Cc: 10592 <at> debbugs.gnu.org,  655118 <at> bugs.debian.org,  655118-forwarded <at> bugs.debian.org,  jmm <at> inutil.org
> Date: Tue, 24 Jan 2012 20:22:52 -0600
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > I think the right fix for this is to declare `error' with the
> > appropriate printf attribute.  Alternatively, you could use variable
> > argument lists and call vprintf instead.
> 
> Would something like this be acceptable, and if not, how would you like
> to see it adjusted?  The patch changes error() to use an ANSI
> declaration, and it relies on the printf format attribute:

This is fine with me, but please use ATTRIBUTE_FORMAT_PRINTF (defined
in src/config.h) instead of a literal __attribute__(...), which is a
GCC-only thing.

Thanks.

Message #22 received at 10592 <at> debbugs.gnu.org (full text, mbox):

From: Rob Browning <rlb <at> defaultvalue.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 10592 <at> debbugs.gnu.org, 655118-forwarded <at> bugs.debian.org, jmm <at> inutil.org,
	655118 <at> bugs.debian.org
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Wed, 25 Jan 2012 21:13:01 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

> This is fine with me, but please use ATTRIBUTE_FORMAT_PRINTF (defined
> in src/config.h) instead of a literal __attribute__(...), which is a
> GCC-only thing.

Will do.

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

Message #25 received at 10592 <at> debbugs.gnu.org (full text, mbox):

From: Rob Browning <rlb <at> defaultvalue.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 10592 <at> debbugs.gnu.org, 655118-forwarded <at> bugs.debian.org, jmm <at> inutil.org,
	655118 <at> bugs.debian.org
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Wed, 25 Jan 2012 21:25:31 -0600

Rob Browning <rlb <at> defaultvalue.org> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> This is fine with me, but please use ATTRIBUTE_FORMAT_PRINTF (defined
>> in src/config.h) instead of a literal __attribute__(...), which is a
>> GCC-only thing.
>
> Will do.

OK, that appears to be newer than 23.3, so I'll probably stick with the
gcc-only changes for now (in Debian).

Thanks again.
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

Message #30 received at 10592-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 10592-done <at> debbugs.gnu.org
Cc: 655118 <at> bugs.debian.org, Rob Browning <rlb <at> defaultvalue.org>
Subject: Re: bug#10592: Bug#655118: Please enabled hardened build flags
Date: Mon, 30 Jan 2012 20:22:03 -0800

I am not observing this problem in the Emacs trunk, with either
GCC 4.6.2 or GCC 4.7.0 20120127 (experimental), when I compile
with -Wformat -Wformat-security.  I suspect the problem has
already been fixed in the trunk in a different way, by
rewriting movemail to use prototypes.  I'm therefore taking
the liberty of marking this bug as fixed in the Emacs bug
database; please feel free to reopen it if I've misunderstood
the situation.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.