diff mbox

asprintf() issue

Message ID CANSoFxueAj7HapmHe3cFjK+1KEo-JoyoxWWSnaXhVwMyxGP8Ag@mail.gmail.com
State New
Headers show

Commit Message

Archie Cobbs May 18, 2015, 1:14 p.m. UTC
On Mon, May 18, 2015 at 5:46 AM, Florian Weimer <fweimer@redhat.com> wrote:
> > IMO we should be conservative and do (c), and document in NEWS, Release wiki
> > page, and hopefully the manual.
>
> I don't think this is worth the cost.  (Even such little changes add up
> and eventually impact linking time and code size.)  It does not even fix
> a bug, and application code can easily set *ptr to NULL before calling
> asprintf, to get uniform behavior across all known implementations (if
> that simplifies application code).

This makes sense to me as well. What about this then:

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



-Archie

Comments

Joseph Myers May 18, 2015, 1:59 p.m. UTC | #1
On Mon, 18 May 2015, Archie Cobbs wrote:

> On Mon, May 18, 2015 at 5:46 AM, Florian Weimer <fweimer@redhat.com> wrote:
> > > IMO we should be conservative and do (c), and document in NEWS, Release wiki
> > > page, and hopefully the manual.
> >
> > I don't think this is worth the cost.  (Even such little changes add up
> > and eventually impact linking time and code size.)  It does not even fix
> > a bug, and application code can easily set *ptr to NULL before calling
> > asprintf, to get uniform behavior across all known implementations (if
> > that simplifies application code).
> 
> This makes sense to me as well. What about this then:

Even if you don't do (c) old symbol version behaves as now, are you sure 
about doing (a) no new symbol version, rather than (b) new symbol version, 
both are aliases, so new binaries won't run with old glibc?
Archie Cobbs May 18, 2015, 2:12 p.m. UTC | #2
On Mon, May 18, 2015 at 8:59 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Mon, 18 May 2015, Archie Cobbs wrote:
>
>> On Mon, May 18, 2015 at 5:46 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> > > IMO we should be conservative and do (c), and document in NEWS, Release wiki
>> > > page, and hopefully the manual.
>> >
>> > I don't think this is worth the cost.  (Even such little changes add up
>> > and eventually impact linking time and code size.)  It does not even fix
>> > a bug, and application code can easily set *ptr to NULL before calling
>> > asprintf, to get uniform behavior across all known implementations (if
>> > that simplifies application code).
>>
>> This makes sense to me as well. What about this then:
>
> Even if you don't do (c) old symbol version behaves as now, are you sure
> about doing (a) no new symbol version, rather than (b) new symbol version,
> both are aliases, so new binaries won't run with old glibc?

Yes I also agree (b) would be safer... but being new here I don't know
how to create symbol 'aliases' so either someone else will have to do
it (or explain to me how :)

Thanks,
-Archie
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index b7f3c61..03be78a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@ 
+2015-05-18  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-18  Siddhesh Poyarekar  <siddhesh@redhat.com>

     * .gitignore: Ignore generated *.pyc.
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..b9ea0ae 100644
--- a/manual/stdio.texi
+++ b/manual/stdio.texi
@@ -2551,6 +2551,13 @@  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}.
+
+@strong{Compatibility Note:} In versions of @theglibc{} prior to 2.22,
+*@var{ptr} is unmodified if an error occurs.  To ensure consistent
+behavior across all versions, callers should initialize *@var{ptr}
+to NULL prior to invoking {@code asprintf}.
+
 Here is how to use @code{asprintf} to get the same result as the
 @code{snprintf} example, but more easily: