Discussion:
a bad use of strerror_r
(too old to reply)
Bruno Haible
2016-10-16 16:17:17 UTC
Permalink
Raw Message
Hi,

When I compile a gnulib testdir on a glibc system with "gcc -Wall", I see the
following warnings (among others):

argp-help.c: In function 'argp_failure':
argp-help.c:1878:17: warning: assignment makes pointer from integer without a cast [enabled by default]

This is a dangerous one, because the code assumes a wrong signature of
strerror_r.

To reproduce, it is enough to have the combination of 3 modules:

$ ./gnulib-tool --create-testdir --with-tests --dir=/tmp/testdir strerror_r-posix argp error

When I look at the preprocessed source code for that compilation unit,
I see some more details:

$ gcc ... -E argp-help.c | grep strerror_r
extern char *strerror_r (int __errnum, char *__buf, size_t __buflen)
extern int rpl_strerror_r (int errnum, char *buf, size_t buflen) __attribute__ ((__nonnull__ (2)))
s = rpl_strerror_r (errnum, buf, sizeof buf);


There are two problems:

1) argp-help.c uses STRERROR_R_CHAR_P, which is defined by the macro
AC_FUNC_STRERROR_R, but argp.m4 does not require it (only error.m4 does).

2) When the strerror_r-posix module is in place, the results of
AC_FUNC_STRERROR_R have to be ignored, because they don't reflect the
situation after
#define strerror_r rpl_strerror_r

With the following proposed patches, we arrive at a sane situation without
warning, and

$ gcc ... -E argp-help.c | grep strerror_r
extern char *strerror_r (int __errnum, char *__buf, size_t __buflen)
extern int rpl_strerror_r (int errnum, char *buf, size_t buflen) __attribute__ ((__nonnull__ (2)))
if (rpl_strerror_r (errnum, buf, sizeof buf) == 0)


2016-10-16 Bruno Haible <***@clisp.org>

Make the 'argp' module work without the 'error' module.
* m4/argp.m4 (gl_ARGP): Require AC_FUNC_STRERROR_R.

diff --git a/m4/argp.m4 b/m4/argp.m4
index d36c716..478ec21 100644
--- a/m4/argp.m4
+++ b/m4/argp.m4
@@ -1,4 +1,4 @@
-# argp.m4 serial 14
+# argp.m4 serial 15
dnl Copyright (C) 2003-2016 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -54,6 +54,7 @@ AC_DEFUN([gl_ARGP],
AC_CHECK_DECLS_ONCE([putchar_unlocked])
AC_CHECK_FUNCS_ONCE([flockfile funlockfile])
AC_CHECK_HEADERS_ONCE([features.h linewrap.h])
+ AC_REQUIRE([AC_FUNC_STRERROR_R])
])

dnl argp-parse.c depends on GNU getopt internals, therefore use GNU getopt


2016-10-16 Bruno Haible <***@clisp.org>

Fix conflict between strerror_r-posix module and AC_FUNC_STRERROR_R.
* m4/strerror_r.m4 (gl_FUNC_STRERROR_R): Override the values set by the
AC_FUNC_STRERROR_R macro. Define HAVE_DECL_STRERROR_R_ORIG.
* lib/strerror_r.c: Use HAVE_DECL_STRERROR_R_ORIG instead of
HAVE_DECL_STRERROR_R.

diff --git a/lib/strerror_r.c b/lib/strerror_r.c
index 07a00cf..b1d4cf5 100644
--- a/lib/strerror_r.c
+++ b/lib/strerror_r.c
@@ -40,7 +40,7 @@ extern
#endif
int __xpg_strerror_r (int errnum, char *buf, size_t buflen);

-#elif HAVE_DECL_STRERROR_R && !(__GLIBC__ >= 2 || defined __UCLIBC__ || defined __CYGWIN__)
+#elif HAVE_DECL_STRERROR_R_ORIG && !(__GLIBC__ >= 2 || defined __UCLIBC__ || defined __CYGWIN__)

/* The system's strerror_r function is OK, except that its third argument
is 'int', not 'size_t', or its return type is wrong. */
diff --git a/m4/strerror_r.m4 b/m4/strerror_r.m4
index 2318927..06c51e1 100644
--- a/m4/strerror_r.m4
+++ b/m4/strerror_r.m4
@@ -1,4 +1,4 @@
-# strerror_r.m4 serial 15
+# strerror_r.m4 serial 16
dnl Copyright (C) 2002, 2007-2016 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -18,6 +18,8 @@ AC_DEFUN([gl_FUNC_STRERROR_R],
if test $ac_cv_have_decl_strerror_r = no; then
HAVE_DECL_STRERROR_R=0
fi
+ AC_DEFINE_UNQUOTED([HAVE_DECL_STRERROR_R_ORIG], [$HAVE_DECL_STRERROR_R],
+ [Define to 1 if you have the declaration of 'strerror_r' in the system include files, or to 0 otherwise.])

if test $ac_cv_func_strerror_r = yes; then
if test "$ERRNO_H:$REPLACE_STRERROR_0" = :0; then
@@ -36,6 +38,11 @@ AC_DEFUN([gl_FUNC_STRERROR_R],
REPLACE_STRERROR_R=1
fi
fi
+
+ # Overwrite the findings of AC_FUNC_STRERROR_R (for code that uses that).
+ AC_REQUIRE([AC_FUNC_STRERROR_R])
+ AC_DEFINE([HAVE_DECL_STRERROR_R], [1])
+ AC_DEFINE([STRERROR_R_CHAR_P], [0])
])

# Prerequisites of lib/strerror_r.c.
Paul Eggert
2016-10-20 22:43:40 UTC
Permalink
Raw Message
This patch looks good to me. I'll CC: this to Sergey as he's our argp
expert; if he doesn't respond in a few days, please install into gnulib.
Sergey, the patch is here:

http://lists.gnu.org/archive/html/bug-gnulib/2016-10/msg00069.html
Bruno Haible
2016-10-26 12:02:28 UTC
Permalink
Raw Message
Post by Paul Eggert
This patch looks good to me. I'll CC: this to Sergey as he's our argp
expert; if he doesn't respond in a few days, please install into gnulib.
Haven't heard from Sergey. So I pushed the two patches.

Bruno
--
Im memoriam Matthias Erzberger <http://en.wikipedia.org/wiki/Matthias_Erzberger>
Eric Blake
2016-11-04 16:06:40 UTC
Permalink
Raw Message
Post by Bruno Haible
1) argp-help.c uses STRERROR_R_CHAR_P, which is defined by the macro
AC_FUNC_STRERROR_R, but argp.m4 does not require it (only error.m4 does).
2) When the strerror_r-posix module is in place, the results of
AC_FUNC_STRERROR_R have to be ignored, because they don't reflect the
situation after
#define strerror_r rpl_strerror_r
With the following proposed patches, we arrive at a sane situation without
warning, and
$ gcc ... -E argp-help.c | grep strerror_r
extern char *strerror_r (int __errnum, char *__buf, size_t __buflen)
extern int rpl_strerror_r (int errnum, char *buf, size_t buflen) __attribute__ ((__nonnull__ (2)))
if (rpl_strerror_r (errnum, buf, sizeof buf) == 0)
Fix conflict between strerror_r-posix module and AC_FUNC_STRERROR_R.
* m4/strerror_r.m4 (gl_FUNC_STRERROR_R): Override the values set by the
AC_FUNC_STRERROR_R macro. Define HAVE_DECL_STRERROR_R_ORIG.
* lib/strerror_r.c: Use HAVE_DECL_STRERROR_R_ORIG instead of
HAVE_DECL_STRERROR_R.
This commit causes weird compilation failures; in particular, trying to
use it on libvirt causes -Wformat to no longer be used. Why? Because it
Post by Bruno Haible
+
+ # Overwrite the findings of AC_FUNC_STRERROR_R (for code that uses that).
+ AC_REQUIRE([AC_FUNC_STRERROR_R])
+ AC_DEFINE([HAVE_DECL_STRERROR_R], [1])
+ AC_DEFINE([STRERROR_R_CHAR_P], [0])
Symptoms of the failure, as seen in config.log, are as follows:

configure:17696: checking for strerror_r
configure:17696: result: yes
configure:17705: checking whether strerror_r returns char *
configure:17729: gcc -c -g conftest.c >&5
configure:17729: $? = 0
configure:17767: result: yes
configure:17818: checking for external symbol _system_configuration
configure:17836: gcc -o conftest -g conftest.c >&5
conftest.c:216:0: warning: "STRERROR_R_CHAR_P" redefined
#define STRERROR_R_CHAR_P 0

conftest.c:213:0: note: this is the location of the previous definition
#define STRERROR_R_CHAR_P 1

conftest.c:218:27: fatal error: sys/systemcfg.h: No such file or directory
#include <sys/systemcfg.h>
^

which then results in probes for -Werror failing where they used to
succeed, and in libvirt's case this causes a probe for '-Wformat
-Werror' to be incorrectly diagnosed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Loading...