| Message ID | CAFULd4aGQqy9-g6AC-ZrNs3MaSKTB3q58BdbNbqaESSTo5ZtKA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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;
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.