Re: [PATCH, jit]: Robustify vasprintf error checks

Re: [PATCH, jit]: Robustify vasprintf error checks

[Uros Bizjak]

On Fri, Mar 13, 2015 at 6:55 PM, David Malcolm <dmalcolm@redhat.com> wrote:

>> As documented in [1] asprintf and vasprintf return:

> I assume that we can rely that any vasprintf implementation manages on
> failure to at least either write NULL to *ret or to return -1, even if
> some of them fail to do both?

Yes, this is correct. The patch checks both ways. Please also note
that linux manpages claim that in case of error, *ret will be
undefined, while FreeBSD implementation will set *ret to NULL on
error.

> OK for trunk.

Thanks, commited.

Uros.

Re: [PATCH, jit]: Robustify vasprintf error checks

[David Malcolm]

On Fri, 2015-03-13 at 17:53 +0100, Uros Bizjak wrote:
> Hello!
> 
> As documented in [1] asprintf and vasprintf return:
> 
> --quote--
> Return value:
> 
> Both functions set *ret to be a pointer to a malloc()'d buffer
> sufficiently large to hold the formatted string. This pointer should
> be passed to free() to release the allocated storage when it is no
> longer needed.
> 
> The integer value returned by these functions is the number of
> characters that were output to the newly allocated string (excluding
> the final '\0'). To put it differently, the return value will match
> that of strlen(*ret).
> 
> Upon failure, the returned value will be -1, and *ret will be set to NULL.
> 
> Note: Upon failure, other implementations may forget to set *ret and
> leave it in an undefined state. Some other implementations may always
> set *ret upon failure but forget to assign -1 for the return value in
> some edge cases.
> --/quote--
> 
> Based on the note above, the attached patch robustifies vasprintf
> return value checks in jit/jit-recording.c. Actually, the same checks
> are already implemented in function oprint, around line 1655 in
> gengtype.c.

> 2015-02-25  Uros Bizjak  <ubizjak@gmail.com>
> 
>     * jit-recording.c (dump::write): Also check vasprintf return value.
>     (recording::context::add_error_va): Ditto.
>     (recording::string::from_printf): Ditto.
> 
> The patch was bootstrapped and regression tested on x86_64-linux-gnu.
> 
> OK for mainline?

Thanks.

I assume that we can rely that any vasprintf implementation manages on
failure to at least either write NULL to *ret or to return -1, even if
some of them fail to do both?

OK for trunk.

> 
> [1] http://asprintf.insanecoding.org/
> 
> Uros.

[PATCH, jit]: Robustify vasprintf error checks

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Hello!

As documented in [1] asprintf and vasprintf return:

--quote--
Return value:

Both functions set *ret to be a pointer to a malloc()'d buffer
sufficiently large to hold the formatted string. This pointer should
be passed to free() to release the allocated storage when it is no
longer needed.

The integer value returned by these functions is the number of
characters that were output to the newly allocated string (excluding
the final '\0'). To put it differently, the return value will match
that of strlen(*ret).

Upon failure, the returned value will be -1, and *ret will be set to NULL.

Note: Upon failure, other implementations may forget to set *ret and
leave it in an undefined state. Some other implementations may always
set *ret upon failure but forget to assign -1 for the return value in
some edge cases.
--/quote--

Based on the note above, the attached patch robustifies vasprintf
return value checks in jit/jit-recording.c. Actually, the same checks
are already implemented in function oprint, around line 1655 in
gengtype.c.

2015-02-25  Uros Bizjak  <ubizjak@gmail.com>

    * jit-recording.c (dump::write): Also check vasprintf return value.
    (recording::context::add_error_va): Ditto.
    (recording::string::from_printf): Ditto.

The patch was bootstrapped and regression tested on x86_64-linux-gnu.

OK for mainline?

[1] http://asprintf.insanecoding.org/

Uros.

[-- Attachment #2: j.diff.txt --]
[-- Type: text/plain, Size: 2120 bytes --]

Index: jit-recording.c
===================================================================
--- jit-recording.c	(revision 221423)
+++ jit-recording.c	(working copy)
@@ -77,8 +77,9 @@ dump::~dump ()
 void
 dump::write (const char *fmt, ...)
 {
+  int len;
   va_list ap;
-  char *buf = NULL;
+  char *buf;
 
   /* If there was an error opening the file, we've already reported it.
      Don't attempt further work.  */
@@ -86,10 +87,10 @@ dump::write (const char *fmt, ...)
     return;
 
   va_start (ap, fmt);
-  vasprintf (&buf, fmt, ap);
+  len = vasprintf (&buf, fmt, ap);
   va_end (ap);
 
-  if (!buf)
+  if (buf == NULL || len < 0)
     {
       m_ctxt.add_error (NULL, "malloc failure writing to dumpfile %s",
 			m_filename);
@@ -1231,6 +1232,7 @@ recording::context::add_error (location *loc, cons
 void
 recording::context::add_error_va (location *loc, const char *fmt, va_list ap)
 {
+  int len;
   char *malloced_msg;
   const char *errmsg;
   bool has_ownership;
@@ -1237,16 +1239,16 @@ recording::context::add_error_va (location *loc, c
 
   JIT_LOG_SCOPE (get_logger ());
 
-  vasprintf (&malloced_msg, fmt, ap);
-  if (malloced_msg)
+  len = vasprintf (&malloced_msg, fmt, ap);
+  if (malloced_msg == NULL || len < 0)
     {
-      errmsg = malloced_msg;
-      has_ownership = true;
+      errmsg = "out of memory generating error message";
+      has_ownership = false;
     }
   else
     {
-      errmsg = "out of memory generating error message";
-      has_ownership = false;
+      errmsg = malloced_msg;
+      has_ownership = true;
     }
   if (get_logger ())
     get_logger ()->log ("error %i: %s", m_error_count, errmsg);
@@ -1709,15 +1711,16 @@ recording::string::~string ()
 recording::string *
 recording::string::from_printf (context *ctxt, const char *fmt, ...)
 {
+  int len;
   va_list ap;
-  char *buf = NULL;
+  char *buf;
   recording::string *result;
 
   va_start (ap, fmt);
-  vasprintf (&buf, fmt, ap);
+  len = vasprintf (&buf, fmt, ap);
   va_end (ap);
 
-  if (!buf)
+  if (buf == NULL || len < 0)
     {
       ctxt->add_error (NULL, "malloc failure");
       return NULL;