Re: asprintf() issue

[Archie Cobbs <archie dot cobbs at gmail dot com>]

On Wed, May 13, 2015 at 11:42 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> On 13/05/15 17:14, Joseph Myers wrote:
>> It's not clear to me that we want to guarantee this as an API.  (Though
>> the existence of code relying on it would indicate that if we changed it,
>> we should also change the symbol version and keep a version that doesn't
>> modify it at the old symbol version.  And as I understand that bug report,
>> the code in question would work just as well if glibc changed to set it to
>> NULL, which was previously requested in
>> <https://sourceware.org/ml/libc-alpha/2001-12/msg00021.html>.  I don't see
>> any good reason to do something other than leaving it unchanged or setting
>> it to NULL, however.)
>>
>
> if all bsd set *ptr to 0 on error then glibc should not
> guarantee incompatible behaviour.

I agree it makes even more sense (safer, more compatible, etc.) to
always set it to NULL on error.

I didn't feel confident enough to propose such a change, but
personally would like this outcome even better.

Here's a patch/sketch (I'm new to glibc development so this patch may
miss something important).

 ChangeLog             | 7 +++++++
 argp/argp-help.c      | 6 ++----
 debug/vasprintf_chk.c | 6 +++++-
 libio/vasprintf.c     | 6 +++++-
 manual/stdio.texi     | 2 ++
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 3bf0690..d4b9505 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2015-05-13  Archie Cobbs <archie.cobbs@gmail.com>
+
+    * libio/vasprintf.c: Set pointer to NULL on error.
+    * debug/vasprintf_chk.c: Likewise.
+    * argp/argp-help.c: Expect NULL on error from _IO_vasprintf().
+    * manual/stdio.texi: Specify that *asprintf() sets NULL on error.
+
 2015-05-13  Leonhard Holz  <leonhard.holz@web.de>

     * benchtests/bench-strcoll.c: New benchmark.
diff --git a/argp/argp-help.c b/argp/argp-help.c
index b055e45..8705b76 100644
--- a/argp/argp-help.c
+++ b/argp/argp-help.c
@@ -1769,8 +1769,7 @@ __argp_error (const struct argp_state *state,
const char *fmt, ...)
 #ifdef _LIBC
       char *buf;

-      if (_IO_vasprintf (&buf, fmt, ap) < 0)
-        buf = NULL;
+      _IO_vasprintf (&buf, fmt, ap);

       __fxprintf (stream, "%s: %s\n",
               state ? state->name : __argp_short_program_name (), buf);
@@ -1839,8 +1838,7 @@ __argp_failure (const struct argp_state *state,
int status, int errnum,
 #ifdef _LIBC
           char *buf;

-          if (_IO_vasprintf (&buf, fmt, ap) < 0)
-        buf = NULL;
+          _IO_vasprintf (&buf, fmt, ap);

           __fxprintf (stream, ": %s", buf);

diff --git a/debug/vasprintf_chk.c b/debug/vasprintf_chk.c
index 8ecb9e8..a85d069 100644
--- a/debug/vasprintf_chk.c
+++ b/debug/vasprintf_chk.c
@@ -47,7 +47,10 @@ __vasprintf_chk (char **result_ptr, int flags,
const char *format,
      we know we will never seek on the stream.  */
   string = (char *) malloc (init_string_size);
   if (string == NULL)
-    return -1;
+    {
+      *result_ptr = NULL;
+      return -1;
+    }
 #ifdef _IO_MTSAFE_IO
   sf._sbf._f._lock = NULL;
 #endif
@@ -67,6 +70,7 @@ __vasprintf_chk (char **result_ptr, int flags, const
char *format,
   if (ret < 0)
     {
       free (sf._sbf._f._IO_buf_base);
+      *result_ptr = NULL;
       return ret;
     }
   /* Only use realloc if the size we need is of the same (binary)
diff --git a/libio/vasprintf.c b/libio/vasprintf.c
index 7f9c105..a423409 100644
--- a/libio/vasprintf.c
+++ b/libio/vasprintf.c
@@ -49,7 +49,10 @@ _IO_vasprintf (result_ptr, format, args)
      we know we will never seek on the stream.  */
   string = (char *) malloc (init_string_size);
   if (string == NULL)
-    return -1;
+    {
+      *result_ptr = NULL;
+      return -1;
+    }
 #ifdef _IO_MTSAFE_IO
   sf._sbf._f._lock = NULL;
 #endif
@@ -63,6 +66,7 @@ _IO_vasprintf (result_ptr, format, args)
   if (ret < 0)
     {
       free (sf._sbf._f._IO_buf_base);
+      *result_ptr = NULL;
       return ret;
     }
   /* Only use realloc if the size we need is of the same (binary)
diff --git a/manual/stdio.texi b/manual/stdio.texi
index e407170..251c2bd 100644
--- a/manual/stdio.texi
+++ b/manual/stdio.texi
@@ -2551,6 +2551,8 @@ The return value is the number of characters
allocated for the buffer, or
 less than zero if an error occurred.  Usually this means that the buffer
 could not be allocated.

+If an error occurs, *@var{ptr} is set to @code{NULL}.
+
 Here is how to use @code{asprintf} to get the same result as the
 @code{snprintf} example, but more easily:

-Archie

-- 
Archie L. Cobbs