bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: New module argp-version-etc


From: Eric Blake
Subject: Re: New module argp-version-etc
Date: Thu, 25 Jun 2009 21:45:18 +0000 (UTC)
User-agent: Loom/3.14 (http://gmane.org/)

Sergey Poznyakoff <gray <at> gnu.org.ua> writes:

> I have fixed the issues Bruno pointed out in his posting, and committed
> the following changes.

And introduced a bug to plain version_etc clients in the process, with the 
potential to make --version segfault.  Sorry for not spotting it before you 
committed:

$ build/src/m4 --version | tail -n2
Written by Rene' Seindal, (null), Eric Blake,
E|!8D, and (null).

> +  for (n_authors = 0;
> +       n_authors < 10
> +      && (authtab[n_authors++] = va_arg (authors, const char *)) != NULL;
> +       n_authors++)

Oops.  n_authors is incremented twice per iteration, so every other authtab 
entry is uninitialized memory, and the trailing NULL is treated as an author.

Also, calling:

version_etc(..., AUTHORS, 0)

is a bug, as is:

version_etc(..., AUTHORS, NULL)

on platforms not compliant to POSIX [POSIX 2001 and C99 state that '#define 
NULL 0' is okay; it isn't until POSIX 2008 that you get the additional 
restriction that NULL must be equivalent to ((void*)0)].  The bug occurs if int 
does not pass through ... the same as a pointer.  Therefore, I'm adding the 
sentinel annotation to the declaration to help catch this via a gcc warning.

Should we make gnulib provide replacement headers for systems that have such a 
poorly defined NULL?  Or is NULL even poorly defined in practice on any of 
gnulib's current set of reasonable porting targets?  It would be much nicer to 
guarantee via gnulib that NULL is always a pointer type than it would be to 
have to audit all clients of varargs functions for proper casting of NULL.  But 
since I don't know the current state of portability, I went ahead and used the 
cast in the test program below.

I will be checking in this patch, if no one else beats me to it.


From: Eric Blake <address@hidden>
Date: Thu, 25 Jun 2009 12:13:35 -0600
Subject: [PATCH] version-etc: fix regression

* lib/version-etc.h (ATTRIBUTE_SENTINEL): Define for new enough
gcc.
(version_etc): Use it, to catch bugs with trailing NULL.
* lib/version-etc.c (version_etc_arn): Delete unused argument.
(version_etc_va): Fix logic bug.
* modules/version-etc-tests: Add test.
* tests/test-version-etc.c: New file.
* tests/test-version-etc.sh: Likewise.
* modules/argp-version-etc-tests (Makefile.am): Only run one
silent test.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                      |   14 ++++++++++++++
 lib/version-etc.c              |    4 ++--
 lib/version-etc.h              |   20 +++++++++++++++-----
 modules/argp-version-etc-tests |    2 +-
 modules/version-etc-tests      |   13 +++++++++++++
 tests/test-version-etc.c       |   33 +++++++++++++++++++++++++++++++++
 tests/test-version-etc.sh      |   40 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 118 insertions(+), 8 deletions(-)
 create mode 100644 modules/version-etc-tests
 create mode 100644 tests/test-version-etc.c
 create mode 100755 tests/test-version-etc.sh

diff --git a/ChangeLog b/ChangeLog
index fc5719b..6970cb3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2009-06-25  Eric Blake  <address@hidden>

+       version-etc: fix regression
+       * lib/version-etc.h (ATTRIBUTE_SENTINEL): Define for new enough
+       gcc.
+       (version_etc): Use it, to catch bugs with trailing NULL.
+       * lib/version-etc.c (version_etc_arn): Delete unused argument.
+       (version_etc_va): Fix logic bug.
+       * modules/version-etc-tests: Add test.
+       * tests/test-version-etc.c: New file.
+       * tests/test-version-etc.sh: Likewise.
+       * modules/argp-version-etc-tests (Makefile.am): Only run one
+       silent test.
+
+2009-06-25  Eric Blake  <address@hidden>
+
        fpurge: migrate into <stdio.h>
        * lib/fpurge.h: Delete...
        * lib/stdio.in.h (fpurge): ...and declare here, instead.
diff --git a/lib/version-etc.c b/lib/version-etc.c
index 9580f53..a580140 100644
--- a/lib/version-etc.c
+++ b/lib/version-etc.c
@@ -161,7 +161,7 @@ Written by %s, %s, %s,\n%s, %s, %s, %s,\n%s, and %s.\n"),
       fprintf (stream, _("\
 Written by %s, %s, %s,\n%s, %s, %s, %s,\n%s, %s, and others.\n"),
                authors[0], authors[1], authors[2], authors[3], authors[4],
-               authors[5], authors[6], authors[7], authors[8], authors[9]);
+               authors[5], authors[6], authors[7], authors[8]);
       break;
     }
 }
@@ -196,7 +196,7 @@ version_etc_va (FILE *stream,

   for (n_authors = 0;
        n_authors < 10
-        && (authtab[n_authors++] = va_arg (authors, const char *)) != NULL;
+        && (authtab[n_authors] = va_arg (authors, const char *)) != NULL;
        n_authors++)
     ;
   version_etc_arn (stream, command_name, package, version,
diff --git a/lib/version-etc.h b/lib/version-etc.h
index 078601c..8ce02fe 100644
--- a/lib/version-etc.h
+++ b/lib/version-etc.h
@@ -22,6 +22,15 @@
 # include <stdarg.h>
 # include <stdio.h>

+/* The `sentinel' attribute was added in gcc 4.0.  */
+#ifndef ATTRIBUTE_SENTINEL
+# if 4 <= __GNUC__
+#  define ATTRIBUTE_SENTINEL __attribute__ ((__sentinel__))
+# else
+#  define ATTRIBUTE_SENTINEL /* empty */
+# endif
+#endif
+
 extern const char version_etc_copyright[];

 /* The three functions below display the --version information in the
@@ -39,28 +48,29 @@ extern const char version_etc_copyright[];

    The functions differ in the way they are passed author names: */

-/* N_AUTHORS names are supplied in array AUTHORS */
+/* N_AUTHORS names are supplied in array AUTHORS.  */
 extern void version_etc_arn (FILE *stream,
                             const char *command_name, const char *package,
                             const char *version,
                             const char * const * authors, size_t n_authors);

-/* Names are passed in the NULL-terminated array AUTHORS */
+/* Names are passed in the NULL-terminated array AUTHORS.  */
 extern void version_etc_ar (FILE *stream,
                            const char *command_name, const char *package,
                            const char *version, const char * const * authors);

-/* Names are passed in the NULL-terminated va_list */
+/* Names are passed in the NULL-terminated va_list.  */
 extern void version_etc_va (FILE *stream,
                            const char *command_name, const char *package,
                            const char *version, va_list authors);

 /* Names are passed as separate arguments, with an additional
-   NULL argument at the end. */
+   NULL argument at the end.  */
 extern void version_etc (FILE *stream,
                         const char *command_name, const char *package,
                         const char *version,
-                        /* const char *author1, ...*/ ...);
+                        /* const char *author1, ..., NULL */ ...)
+  ATTRIBUTE_SENTINEL;

 /* Display the usual `Report bugs to' stanza */
 extern void emit_bug_reporting_address (void);
diff --git a/modules/argp-version-etc-tests b/modules/argp-version-etc-tests
index eeed143..96c5939 100644
--- a/modules/argp-version-etc-tests
+++ b/modules/argp-version-etc-tests
@@ -8,7 +8,7 @@ progname
 version-etc-fsf

 Makefile.am:
-TESTS += test-argp-version-etc test-argp-version-etc-1.sh
+TESTS += test-argp-version-etc-1.sh
 TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@'
 check_PROGRAMS += test-argp-version-etc
 test_argp_version_etc_LDADD = $(LDADD) @LIBINTL@
diff --git a/modules/version-etc-tests b/modules/version-etc-tests
new file mode 100644
index 0000000..94ff3b7
--- /dev/null
+++ b/modules/version-etc-tests
@@ -0,0 +1,13 @@
+Files:
+tests/test-version-etc.c
+tests/test-version-etc.sh
+
+Depends-on:
+progname
+version-etc-fsf
+
+Makefile.am:
+TESTS += test-version-etc.sh
+TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@'
+check_PROGRAMS += test-version-etc
+test_version_etc_LDADD = $(LDADD) @LIBINTL@
diff --git a/tests/test-version-etc.c b/tests/test-version-etc.c
new file mode 100644
index 0000000..71cbc8b
--- /dev/null
+++ b/tests/test-version-etc.c
@@ -0,0 +1,33 @@
+/* Test suite for version-etc.
+   Copyright (C) 2009 Free Software Foundation, Inc.
+   This file is part of the GNUlib Library.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+
+#include "version-etc.h"
+
+#include "progname.h"
+
+#define AUTHORS "Sergey Poznyakoff", "Eric Blake"
+
+int
+main (int argc, char **argv)
+{
+  set_program_name (argv[0]);
+  version_etc (stdout, "test-version-etc", "dummy", "0", AUTHORS,
+               (const char *) NULL);
+  return 0;
+}
diff --git a/tests/test-version-etc.sh b/tests/test-version-etc.sh
new file mode 100755
index 0000000..3b2fc4d
--- /dev/null
+++ b/tests/test-version-etc.sh
@@ -0,0 +1,40 @@
+#! /bin/sh
+# Test suite for version-etc.
+# Copyright (C) 2009 Free Software Foundation, Inc.
+# This file is part of the GNUlib Library.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+TMP=ve-expected.tmp
+LC_ALL=C
+export LC_ALL
+ERR=0
+
+cat > $TMP <<EOT
+test-version-etc (dummy) 0
+COPYRIGHT Free Software Foundation, Inc.
+License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
+This is free software: you are free to change and redistribute it.
+There is NO WARRANTY, to the extent permitted by law.
+
+Written by Sergey Poznyakoff and Eric Blake.
+EOT
+
+./test-version-etc --version |
+ sed '2s/Copyright (C) [0-9]\{4,4\}/COPYRIGHT/' |
+ diff -c $TMP - || ERR=1
+
+rm $TMP
+
+exit $ERR
-- 
1.6.3.2








reply via email to

[Prev in Thread] Current Thread [Next in Thread]