Re: [jit] Eliminate fixed-size buffers used with vsnprintf

[Jeff Law]

On 09/24/14 14:24, Joseph S. Myers wrote:

On Wed, 24 Sep 2014, David Malcolm wrote:


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.


GCC is designed on the basis of aborting on allocation failures - as is
GMP, which allows custom allocation functions to be specified but still
requires them to exit the program rather than return, longjmp or throw an
exception.
But that may be something we need to change if GCC is going to be used 
at a JIT in the future.  Yea, we'll still have problems of this nature 
in libraries that GCC itself might use such as gmp, but that doesn't 
mean that we have to perpetuate that practice in GCC itself.




Presumably I should address this in a followup, by making that be
dynamically-allocated?


Yes.  Arbitrary limits should be avoided in GNU.

Agreed.

jeff

Re: [jit] Eliminate fixed-size buffers used with vsnprintf

[Joseph S. Myers]

On Wed, 24 Sep 2014, David Malcolm wrote:

> 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.

GCC is designed on the basis of aborting on allocation failures - as is 
GMP, which allows custom allocation functions to be specified but still 
requires them to exit the program rather than return, longjmp or throw an 
exception.

> 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?

Yes.  Arbitrary limits should be avoided in GNU.

-- 
Joseph S. Myers
jos...@codesourcery.com

[jit] Eliminate fixed-size buffers used with vsnprintf

[David Malcolm]

On Tue, 2014-09-23 at 23:27 +, 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 -  and 
>  (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  
 
+   * 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  
+
* 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 *mallo