On Tue, 2014-09-23 at 23:27 +0000, Joseph S. Myers wrote:
[...]
> dump::write, recording::context::add_error_va,
> recording::string::from_printf all use fixed-size buffers with vsnprintf
> but no apparent reason to assume this can never result in truncation, and
> missing API comments (lots of functions are missing such comments ...) to
> state either the caller's responsibility to limit the length of the
> result, or that the API provides for truncation. Unless there's a
> definite reason truncation is needed, dynamic allocation should be used.
> A patch was submitted a while back to add xasprintf and xvasprintf to
> libiberty - <https://gcc.gnu.org/ml/gcc-patches/2009-11/msg01448.html> and
> <https://gcc.gnu.org/ml/gcc-patches/2009-11/msg01449.html> (I don't know
> if that's the most recent version), which could be resurrected.
[...]
The ideal I'm aiming for here is that a well-behaved library should
never abort, so I've rewritten these functions to use vasprintf, and
added error-handling checks to cover the case where malloc returns NULL
within vasprintf.
I believe this fixes the specific issues you pointed out (apart from the
numerous missing API comments, which I'll do it a followup). Note that
there's still a fixed-size buffer within gcc::jit::recording::context,
the field:
char m_first_error_str[1024];
Currently this is populated using strncpy followed by an explicit write
of a truncation byte to make sure, but it *is* another truncation.
Presumably I should address this in a followup, by making that be
dynamically-allocated?
(perhaps by making errors be first-class entities in the API, by
introducing a gcc_jit_error subclass of gcc_jit_object, with a location
field as well as the text message).
Committed to branch dmalcolm/jit:
gcc/jit/ChangeLog.jit:
* internal-api.c (gcc::jit::dump::write): Eliminate fixed-size
buffer "buf" by replacing call to vsnprintf with one to vasprintf
and a free, emitting an error on the dump's context if a malloc
failure occurs.
(gcc::jit::recording::context::add_error_va): Likewise, using
a precanned message if the malloc inside vasprinf fails. Split
local "buf" into "malloced_msg" and "errmsg" to ensure that we
free the message iff we're using one malloc-ed by vasprintf.
(gcc::jit::recording::string::from_printf): Eliminate fixed-size
buffer "buf" by replacing call to vsnprintf with one to vasprintf
and a free, emitting an error on the relevant context if a malloc
failure occurs.
---
gcc/jit/ChangeLog.jit | 15 +++++++++++++++
gcc/jit/internal-api.c | 50 ++++++++++++++++++++++++++++++++++++++------------
2 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/gcc/jit/ChangeLog.jit b/gcc/jit/ChangeLog.jit
index 4ddd3cb..3cadaab 100644
--- a/gcc/jit/ChangeLog.jit
+++ b/gcc/jit/ChangeLog.jit
@@ -1,5 +1,20 @@
2014-09-24 David Malcolm <[email protected]>
+ * internal-api.c (gcc::jit::dump::write): Eliminate fixed-size
+ buffer "buf" by replacing call to vsnprintf with one to vasprintf
+ and a free, emitting an error on the dump's context if a malloc
+ failure occurs.
+ (gcc::jit::recording::context::add_error_va): Likewise, using
+ a precanned message if the malloc inside vasprinf fails. Split
+ local "buf" into "malloced_msg" and "errmsg" to ensure that we
+ free the message iff we're using one malloc-ed by vasprintf.
+ (gcc::jit::recording::string::from_printf): Eliminate fixed-size
+ buffer "buf" by replacing call to vsnprintf with one to vasprintf
+ and a free, emitting an error on the relevant context if a malloc
+ failure occurs.
+
+2014-09-24 David Malcolm <[email protected]>
+
* dummy-frontend.c: Update copyright year. Follow standard for
initial includes by removing redundant include of "ansidecl.h".
* internal-api.c: Follow standard for initial includes by removing
diff --git a/gcc/jit/internal-api.c b/gcc/jit/internal-api.c
index 76ada70..15e9f81 100644
--- a/gcc/jit/internal-api.c
+++ b/gcc/jit/internal-api.c
@@ -95,12 +95,20 @@ dump::~dump ()
void
dump::write (const char *fmt, ...)
{
- char buf[4096];
va_list ap;
+ char *buf = NULL;
+
va_start (ap, fmt);
- vsnprintf (buf, sizeof (buf), fmt, ap);
+ vasprintf (&buf, fmt, ap);
va_end (ap);
+ if (!buf)
+ {
+ m_ctxt.add_error (NULL, "malloc failure writing to dumpfile %s",
+ m_filename);
+ return;
+ }
+
fwrite (buf, strlen (buf), 1, m_file);
/* Update line/column: */
@@ -114,6 +122,8 @@ dump::write (const char *fmt, ...)
else
m_column++;
}
+
+ free (buf);
}
recording::location *
@@ -680,8 +690,14 @@ recording::context::add_error (location *loc, const char
*fmt, ...)
void
recording::context::add_error_va (location *loc, const char *fmt, va_list ap)
{
- char buf[1024];
- vsnprintf (buf, sizeof (buf) - 1, fmt, ap);
+ char *malloced_msg;
+ const char *errmsg;
+
+ vasprintf (&malloced_msg, fmt, ap);
+ if (malloced_msg)
+ errmsg = malloced_msg;
+ else
+ errmsg = "out of memory generating error message";
const char *ctxt_progname =
get_str_option (GCC_JIT_STR_OPTION_PROGNAME);
@@ -692,19 +708,21 @@ recording::context::add_error_va (location *loc, const
char *fmt, va_list ap)
fprintf (stderr, "%s: %s: error: %s\n",
ctxt_progname,
loc->get_debug_string (),
- buf);
+ errmsg);
else
fprintf (stderr, "%s: error: %s\n",
ctxt_progname,
- buf);
+ errmsg);
if (!m_error_count)
{
- strncpy (m_first_error_str, buf, sizeof(m_first_error_str));
+ strncpy (m_first_error_str, errmsg, sizeof(m_first_error_str));
m_first_error_str[sizeof(m_first_error_str) - 1] = '\0';
}
m_error_count++;
+
+ free (malloced_msg);
}
const char *
@@ -797,15 +815,23 @@ recording::string::~string ()
recording::string *
recording::string::from_printf (context *ctxt, const char *fmt, ...)
{
- char buf[4096];
va_list ap;
- va_start (ap, fmt);
-
- vsnprintf (buf, sizeof (buf), fmt, ap);
+ char *buf = NULL;
+ recording::string *result;
+ va_start (ap, fmt);
+ vasprintf (&buf, fmt, ap);
va_end (ap);
- return ctxt->new_string (buf);
+ if (!buf)
+ {
+ ctxt->add_error (NULL, "malloc failure");
+ return NULL;
+ }
+
+ result = ctxt->new_string (buf);
+ free (buf);
+ return result;
}
recording::string *
--
1.7.11.7
