[PATCH] Replace potentially unsafe alloca with xmalloc/xfree in value_concat

[PATCH] Replace potentially unsafe alloca with xmalloc/xfree in value_concat

[Siddhesh Poyarekar]

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

Hi,

Here is another instance of an alloca that may not be safe.

The value_concat function allocates space on stack to concatenate two
strings (or duplicate a string/char/bool n times). This is not safe
because the requested space could cause alloca to move the stack
pointer beyond stack boundary. Attached patch replaces the alloca with
xmalloc/xfree.

I don't have a test case to demonstrate this, but I think the only
language this can probably be demonstrated in is ADA, a language that
I have never used. I have however verified that this does not cause any
regressions in the testsuite. OK to commit?

Regards,
Siddhesh

gdb/ChangeLog:

	* valarith.c (value_concat): Replace unsafe ALLOCA with
	XMALLOC/XFREE.

[-- Attachment #2: unsafe-alloca.patch --]
[-- Type: text/x-patch, Size: 1834 bytes --]

? unsafe-alloca.patch
Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.105
diff -u -r1.105 valarith.c
--- gdb/valarith.c	16 Aug 2012 07:36:20 -0000	1.105
+++ gdb/valarith.c	14 Sep 2012 08:31:38 -0000
@@ -668,9 +668,11 @@
       if (TYPE_CODE (type2) == TYPE_CODE_STRING
 	  || TYPE_CODE (type2) == TYPE_CODE_CHAR)
 	{
+	  struct cleanup *back_to;
 	  count = longest_to_int (value_as_long (inval1));
 	  inval2len = TYPE_LENGTH (type2);
-	  ptr = (char *) alloca (count * inval2len);
+	  ptr = (char *) xmalloc (count * inval2len);
+	  back_to = make_cleanup (xfree, ptr);
 	  if (TYPE_CODE (type2) == TYPE_CODE_CHAR)
 	    {
 	      char_type = type2;
@@ -693,6 +695,7 @@
 		}
 	    }
 	  outval = value_string (ptr, count * inval2len, char_type);
+	  do_cleanups (back_to);
 	}
       else if (TYPE_CODE (type2) == TYPE_CODE_BOOL)
 	{
@@ -706,6 +709,8 @@
   else if (TYPE_CODE (type1) == TYPE_CODE_STRING
 	   || TYPE_CODE (type1) == TYPE_CODE_CHAR)
     {
+      struct cleanup *back_to;
+
       /* We have two character strings to concatenate.  */
       if (TYPE_CODE (type2) != TYPE_CODE_STRING
 	  && TYPE_CODE (type2) != TYPE_CODE_CHAR)
@@ -714,7 +719,8 @@
 	}
       inval1len = TYPE_LENGTH (type1);
       inval2len = TYPE_LENGTH (type2);
-      ptr = (char *) alloca (inval1len + inval2len);
+      ptr = (char *) xmalloc (inval1len + inval2len);
+      back_to = make_cleanup (xfree, ptr);
       if (TYPE_CODE (type1) == TYPE_CODE_CHAR)
 	{
 	  char_type = type1;
@@ -737,6 +743,7 @@
 	  memcpy (ptr + inval1len, value_contents (inval2), inval2len);
 	}
       outval = value_string (ptr, inval1len + inval2len, char_type);
+      do_cleanups (back_to);
     }
   else if (TYPE_CODE (type1) == TYPE_CODE_BOOL)
     {

Re: [PATCH] Replace potentially unsafe alloca with xmalloc/xfree in value_concat

[Jan Kratochvil]

Hi Siddhesh,

On Fri, 14 Sep 2012 11:16:29 +0200, Siddhesh Poyarekar wrote:
> --- gdb/valarith.c	16 Aug 2012 07:36:20 -0000	1.105
> +++ gdb/valarith.c	14 Sep 2012 08:31:38 -0000
> @@ -668,9 +668,11 @@
>        if (TYPE_CODE (type2) == TYPE_CODE_STRING
>  	  || TYPE_CODE (type2) == TYPE_CODE_CHAR)
>  	{
> +	  struct cleanup *back_to;

Empty line after declarations.


>  	  count = longest_to_int (value_as_long (inval1));
>  	  inval2len = TYPE_LENGTH (type2);
> -	  ptr = (char *) alloca (count * inval2len);
> +	  ptr = (char *) xmalloc (count * inval2len);
> +	  back_to = make_cleanup (xfree, ptr);


OK with that change.


Thanks,
Jan

Re: [PATCH] Replace potentially unsafe alloca with xmalloc/xfree in value_concat

[Siddhesh Poyarekar]

On Fri, 14 Sep 2012 13:37:05 +0200, Jan wrote:
> OK with that change.

Thanks, committed.

Regards,
Siddhesh

http://sourceware.org/ml/gdb-cvs/2012-09/msg00068.html

CVSROOT:	/cvs/src
Module name:	src
Changes by:	siddhesh@sourceware.org	2012-09-14 12:46:56

Modified files:
	gdb            : ChangeLog valarith.c 

Log message:
	* valarith.c (value_concat): Replace unsafe ALLOCA with
	XMALLOC/XFREE.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/ChangeLog.diff?cvsroot=src&r1=1.14659&r2=1.14660
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/valarith.c.diff?cvsroot=src&r1=1.105&r2=1.106

Re: [PATCH] Replace potentially unsafe alloca with xmalloc/xfree in value_concat

[Jan Kratochvil]

On Fri, 14 Sep 2012 14:48:33 +0200, Siddhesh Poyarekar wrote:
> http://sourceware.org/ml/gdb-cvs/2012-09/msg00068.html
> 
> CVSROOT:	/cvs/src
> Module name:	src
> Changes by:	siddhesh@sourceware.org	2012-09-14 12:46:56
> 
> Modified files:
> 	gdb            : ChangeLog valarith.c 
> 
> Log message:
> 	* valarith.c (value_concat): Replace unsafe ALLOCA with
> 	XMALLOC/XFREE.
> 
> Patches:
> http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/ChangeLog.diff?cvsroot=src&r1=1.14659&r2=1.14660
> http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/valarith.c.diff?cvsroot=src&r1=1.105&r2=1.106

I understand it is not significant but:

In this case you could post just the first link as this patch itself has been
(in fact - one line change) already posted here before.

If you meant this as the patch post this is still not easily + offline
reviewable.  For a full commit post either post 'cvs diff' before you commit
it or wait half an hour for mirror into GIT and take it from GIT or use the
script of mine:
	http://git.jankratochvil.net/?p=nethome.git;a=blob_plain;f=bin/changelogget;hb=master
	->
	changelogget -n http://sourceware.org/ml/gdb-cvs/2012-09/msg00068.html

I do not know how other people do that, there are many various possibilities.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2012-09/msg00068.html

--- src/gdb/ChangeLog	2012/09/14 12:10:21	1.14659
+++ src/gdb/ChangeLog	2012/09/14 12:46:55	1.14660
@@ -1,3 +1,8 @@
+2012-09-14  Siddhesh Poyarekar  <siddhesh@redhat.com>
+
+	* valarith.c (value_concat): Replace unsafe ALLOCA with
+	XMALLOC/XFREE.
+
 2012-09-14  Pedro Alves  <palves@redhat.com>
 
 	* gdb.1 (SEE ALSO): Expand pointer to GDB's Texinfo manual.
--- src/gdb/valarith.c	2012/08/16 07:36:20	1.105
+++ src/gdb/valarith.c	2012/09/14 12:46:55	1.106
@@ -668,9 +668,12 @@
       if (TYPE_CODE (type2) == TYPE_CODE_STRING
 	  || TYPE_CODE (type2) == TYPE_CODE_CHAR)
 	{
+	  struct cleanup *back_to;
+
 	  count = longest_to_int (value_as_long (inval1));
 	  inval2len = TYPE_LENGTH (type2);
-	  ptr = (char *) alloca (count * inval2len);
+	  ptr = (char *) xmalloc (count * inval2len);
+	  back_to = make_cleanup (xfree, ptr);
 	  if (TYPE_CODE (type2) == TYPE_CODE_CHAR)
 	    {
 	      char_type = type2;
@@ -693,6 +696,7 @@
 		}
 	    }
 	  outval = value_string (ptr, count * inval2len, char_type);
+	  do_cleanups (back_to);
 	}
       else if (TYPE_CODE (type2) == TYPE_CODE_BOOL)
 	{
@@ -706,6 +710,8 @@
   else if (TYPE_CODE (type1) == TYPE_CODE_STRING
 	   || TYPE_CODE (type1) == TYPE_CODE_CHAR)
     {
+      struct cleanup *back_to;
+
       /* We have two character strings to concatenate.  */
       if (TYPE_CODE (type2) != TYPE_CODE_STRING
 	  && TYPE_CODE (type2) != TYPE_CODE_CHAR)
@@ -714,7 +720,8 @@
 	}
       inval1len = TYPE_LENGTH (type1);
       inval2len = TYPE_LENGTH (type2);
-      ptr = (char *) alloca (inval1len + inval2len);
+      ptr = (char *) xmalloc (inval1len + inval2len);
+      back_to = make_cleanup (xfree, ptr);
       if (TYPE_CODE (type1) == TYPE_CODE_CHAR)
 	{
 	  char_type = type1;
@@ -737,6 +744,7 @@
 	  memcpy (ptr + inval1len, value_contents (inval2), inval2len);
 	}
       outval = value_string (ptr, inval1len + inval2len, char_type);
+      do_cleanups (back_to);
     }
   else if (TYPE_CODE (type1) == TYPE_CODE_BOOL)
     {