[RFC] Ada ARI fix for sprintf

[RFC] Ada ARI fix for sprintf

[Pierre Muller]

The reason this is a RFC rather than a RFA
is that I used another function than
the one recommended on ARI page.

Currently AR Index gives 153 failures for "sprint" rule.
sprintf	153	Do not use sprintf, instead use xstrprintf
But they seem to all have local static buffers
whereas xstrprintf allocates memory globally.

  I propose to use xsnprintf instead for such case,
which checks for buffer overflow.

  If you agree, I will also update the
ARI recommendation.

All comments welcome,


Pierre Muller
Pascal language support maintainer for GDB




2009-04-07  Pierre Muller  <muller.u-strasbg.fr²>

	ARI fix: sprintf rule.
	* ada-exp.y (convert_char_literal): Replace sprintf by xsnprintf.
	* ada-lang.c (add_angle_bracket): Ditto.
	(ada_decode, find_old_style_renaming_symbol): Ditto.
	(ada_to_fixed_type_1, ada_enum_name): Ditto.

Index: ada-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/ada-exp.y,v
retrieving revision 1.36
diff -u -p -r1.36 ada-exp.y
--- ada-exp.y	24 Mar 2009 02:08:23 -0000	1.36
+++ ada-exp.y	7 Apr 2009 08:01:25 -0000
@@ -1452,7 +1452,7 @@ convert_char_literal (struct type *type,
 
   if (type == NULL || TYPE_CODE (type) != TYPE_CODE_ENUM)
     return val;
-  sprintf (name, "QU%02x", (int) val);
+  xsnprintf (name, sizeof (name), "QU%02x", (int) val);
   for (f = 0; f < TYPE_NFIELDS (type); f += 1)
     {
       if (strcmp (name, TYPE_FIELD_NAME (type, f)) == 0)
Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.205
diff -u -p -r1.205 ada-lang.c
--- ada-lang.c	24 Mar 2009 02:07:06 -0000	1.205
+++ ada-lang.c	7 Apr 2009 08:01:28 -0000
@@ -335,11 +335,13 @@ static char *
 add_angle_brackets (const char *str)
 {
   static char *result = NULL;
+  int result_size;
 
   xfree (result);
-  result = (char *) xmalloc ((strlen (str) + 3) * sizeof (char));
+  result_size = (strlen (str) + 3) * sizeof (char);
+  result = (char *) xmalloc (result_size);
 
-  sprintf (result, "<%s>", str);
+  xsnprintf (result, result_size, "<%s>", str);
   return result;
 }
 
@@ -1114,7 +1116,7 @@ Suppress:
   if (encoded[0] == '<')
     strcpy (decoded, encoded);
   else
-    sprintf (decoded, "<%s>", encoded);
+    xsnprintf (decoded, decoding_buffer_size, "<%s>", encoded);
   return decoded;
 
 }
@@ -6609,13 +6611,14 @@ find_old_style_renaming_symbol (const ch
         function_name = function_name + 5;
 
       rename = (char *) alloca (rename_len * sizeof (char));
-      sprintf (rename, "%s__%s___XR", function_name, name);
+      xsnprintf (rename, rename_len * sizeof (char), "%s__%s___XR", 
+		 function_name, name);
     }
   else
     {
       const int rename_len = strlen (name) + 6;
       rename = (char *) alloca (rename_len * sizeof (char));
-      sprintf (rename, "%s___XR", name);
+      xsnprintf (rename, rename_len * sizeof (char), "%s___XR", name);
     }
 
   return ada_find_any_symbol (rename);
@@ -7308,7 +7311,7 @@ ada_to_fixed_type_1 (struct type *type, 
             int xvz_found = 0;
             LONGEST size;
 
-            sprintf (xvz_name, "%s___XVZ", name);
+            xsnprintf (xvz_name, strlen (name) + 7, "%s___XVZ", name);
             size = get_int_var_value (xvz_name, &xvz_found);
             if (xvz_found && TYPE_LENGTH (fixed_record_type) != size)
               {
@@ -7760,11 +7763,11 @@ ada_enum_name (const char *name)
 
       GROW_VECT (result, result_len, 16);
       if (isascii (v) && isprint (v))
-        sprintf (result, "'%c'", v);
+        xsnprintf (result, result_len, "'%c'", v);
       else if (name[1] == 'U')
-        sprintf (result, "[\"%02x\"]", v);
+        xsnprintf (result, result_len, "[\"%02x\"]", v);
       else
-        sprintf (result, "[\"%04x\"]", v);
+        xsnprintf (result, result_len, "[\"%04x\"]", v);
 
       return result;
     }

Re: [RFC] Ada ARI fix for sprintf

[Pedro Alves]

On Tuesday 07 April 2009 09:09:27, Pierre Muller wrote:
> The reason this is a RFC rather than a RFA
> is that I used another function than
> the one recommended on ARI page.

> Currently AR Index gives 153 failures for "sprint" rule.
> sprintf	153	Do not use sprintf, instead use xstrprintf
> But they seem to all have local static buffers
> whereas xstrprintf allocates memory globally.

Well, you don't have to follow those rules literally.
The important point is in not using a function that
can overflow, like sprintf.

> 	* ada-exp.y (convert_char_literal): Replace sprintf by xsnprintf.
> 	* ada-lang.c (add_angle_bracket): Ditto.
                                       ^ typo, add_angle_brackets.

> 	(ada_decode, find_old_style_renaming_symbol): Ditto.
> 	(ada_to_fixed_type_1, ada_enum_name): Ditto.
> 

> Index: ada-lang.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ada-lang.c,v
> retrieving revision 1.205
> diff -u -p -r1.205 ada-lang.c
> --- ada-lang.c	24 Mar 2009 02:07:06 -0000	1.205
> +++ ada-lang.c	7 Apr 2009 08:01:28 -0000
> @@ -335,11 +335,13 @@ static char *
>  add_angle_brackets (const char *str)
>  {
>    static char *result = NULL;
> +  int result_size;
>  
>    xfree (result);
> -  result = (char *) xmalloc ((strlen (str) + 3) * sizeof (char));
> +  result_size = (strlen (str) + 3) * sizeof (char);
> +  result = (char *) xmalloc (result_size);
>  
> -  sprintf (result, "<%s>", str);
> +  xsnprintf (result, result_size, "<%s>", str);
>    return result;
>  }

This function is already allocating the buffer from the heap, so,
if you used xstrprintf, the code would become simpler, as you
would get rid of the xmalloc, and the need to compute result_size
yourself.  Something like:

 static char *
 add_angle_brackets (const char *str)
 {
   static char *result = NULL;
 
   xfree (result);
   result = xstrprintf ("<%s>", str);
   return result;
 }

Although, this function is used in the symbol completer, and
I don't know if there's a speed difference in using
xstrprintf over xmalloc+snprintf, and if it makes a difference
in this case.  Probably not somethind to worry about
though...  Joel?

All other cases looked good to me.

-- 
Pedro Alves

Re: [RFC] Ada ARI fix for sprintf

[Joel Brobecker]

> Although, this function is used in the symbol completer, and
> I don't know if there's a speed difference in using
> xstrprintf over xmalloc+snprintf, and if it makes a difference
> in this case.  Probably not somethind to worry about
> though...  Joel?

Agreed - I haven't seen any performance issue in that part of the code,
so better code is great.

-- 
Joel

[RFA] Ada ARI fix for sprintf version 2

[Pierre Muller]

Here is an updated patch for ada sources
removing sprintf usage.
Thanks to Pedro and Joel for the feedback.


 Joel, is this OK?



Pierre Muller
Pascal language support maintainer for GDB




2009-04-14  Pierre Muller  <muller@ics.u-strasbg.fr>

	ARI fix: sprintf rule.
	* ada-exp.y (convert_char_literal): Replace sprintf by xsnprintf.
	* ada-lang.c (add_angle_brackets): Use xstrprintf.
	(ada_decode): Replace sprintf by xsnprintf.
	(find_old_style_renaming_symbol): Ditto.
	(ada_to_fixed_type_1, ada_enum_name): Ditto.



Index: ada-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/ada-exp.y,v
retrieving revision 1.36
diff -u -p -r1.36 ada-exp.y
--- ada-exp.y	24 Mar 2009 02:08:23 -0000	1.36
+++ ada-exp.y	14 Apr 2009 18:34:03 -0000
@@ -1452,7 +1452,7 @@ convert_char_literal (struct type *type,
 
   if (type == NULL || TYPE_CODE (type) != TYPE_CODE_ENUM)
     return val;
-  sprintf (name, "QU%02x", (int) val);
+  xsnprintf (name, sizeof (name), "QU%02x", (int) val);
   for (f = 0; f < TYPE_NFIELDS (type); f += 1)
     {
       if (strcmp (name, TYPE_FIELD_NAME (type, f)) == 0)
Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.205
diff -u -p -r1.205 ada-lang.c
--- ada-lang.c	24 Mar 2009 02:07:06 -0000	1.205
+++ ada-lang.c	14 Apr 2009 18:34:06 -0000
@@ -337,9 +337,7 @@ add_angle_brackets (const char *str)
   static char *result = NULL;
 
   xfree (result);
-  result = (char *) xmalloc ((strlen (str) + 3) * sizeof (char));
-
-  sprintf (result, "<%s>", str);
+  result = xstrprintf ("<%s>", str);
   return result;
 }
 
@@ -1114,7 +1112,7 @@ Suppress:
   if (encoded[0] == '<')
     strcpy (decoded, encoded);
   else
-    sprintf (decoded, "<%s>", encoded);
+    xsnprintf (decoded, decoding_buffer_size, "<%s>", encoded);
   return decoded;
 
 }
@@ -6609,13 +6607,14 @@ find_old_style_renaming_symbol (const ch
         function_name = function_name + 5;
 
       rename = (char *) alloca (rename_len * sizeof (char));
-      sprintf (rename, "%s__%s___XR", function_name, name);
+      xsnprintf (rename, rename_len * sizeof (char), "%s__%s___XR", 
+		 function_name, name);
     }
   else
     {
       const int rename_len = strlen (name) + 6;
       rename = (char *) alloca (rename_len * sizeof (char));
-      sprintf (rename, "%s___XR", name);
+      xsnprintf (rename, rename_len * sizeof (char), "%s___XR", name);
     }
 
   return ada_find_any_symbol (rename);
@@ -7308,7 +7307,7 @@ ada_to_fixed_type_1 (struct type *type, 
             int xvz_found = 0;
             LONGEST size;
 
-            sprintf (xvz_name, "%s___XVZ", name);
+            xsnprintf (xvz_name, strlen (name) + 7, "%s___XVZ", name);
             size = get_int_var_value (xvz_name, &xvz_found);
             if (xvz_found && TYPE_LENGTH (fixed_record_type) != size)
               {
@@ -7760,11 +7759,11 @@ ada_enum_name (const char *name)
 
       GROW_VECT (result, result_len, 16);
       if (isascii (v) && isprint (v))
-        sprintf (result, "'%c'", v);
+        xsnprintf (result, result_len, "'%c'", v);
       else if (name[1] == 'U')
-        sprintf (result, "[\"%02x\"]", v);
+        xsnprintf (result, result_len, "[\"%02x\"]", v);
       else
-        sprintf (result, "[\"%04x\"]", v);
+        xsnprintf (result, result_len, "[\"%04x\"]", v);
 
       return result;
     }

Re: [RFA] Ada ARI fix for sprintf version 2

[Joel Brobecker]

> 2009-04-14  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	ARI fix: sprintf rule.
> 	* ada-exp.y (convert_char_literal): Replace sprintf by xsnprintf.
> 	* ada-lang.c (add_angle_brackets): Use xstrprintf.
> 	(ada_decode): Replace sprintf by xsnprintf.
> 	(find_old_style_renaming_symbol): Ditto.
> 	(ada_to_fixed_type_1, ada_enum_name): Ditto.

Yes, this looks fine to me.

Thanks for doing this.
-- 
Joel

RE: [RFA] Ada ARI fix for sprintf version 2

[Pierre Muller]

Thanks,

patch committed.

Pierre

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Tuesday, April 14, 2009 8:56 PM
> À : Pierre Muller
> Cc : 'Pedro Alves'; gdb-patches@sourceware.org
> Objet : Re: [RFA] Ada ARI fix for sprintf version 2
> 
> > 2009-04-14  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	ARI fix: sprintf rule.
> > 	* ada-exp.y (convert_char_literal): Replace sprintf by xsnprintf.
> > 	* ada-lang.c (add_angle_brackets): Use xstrprintf.
> > 	(ada_decode): Replace sprintf by xsnprintf.
> > 	(find_old_style_renaming_symbol): Ditto.
> > 	(ada_to_fixed_type_1, ada_enum_name): Ditto.
> 
> Yes, this looks fine to me.
> 
> Thanks for doing this.
> --
> Joel