[patch RFC] Fix PR binutils/2584

[patch RFC] Fix PR binutils/2584

[Kaz Kojima]

Hi,

The attached patch is to fix PR binutils/2584:

http://sourceware.org/bugzilla/show_bug.cgi?id=2584

It seems that bfd/tekhex.c:getsym() causes that as described in
the trail #2 of the PR.  The patch attached seems to fix the given
test case in PR and is regtested on i686-pc-linux-gnu with no new
failures, though it'd be better to add more checks.

Regards,
	kaz
--
2006-04-26  Kaz Kojima  <kkojima@rr.iij4u.or.jp>

	PR 2584
	* tekhex.c (first_phase): Change return type to bfd_boolean
	and return false if the unexpected character is found.
	(pass_over): Change return type to bfd_boolean and the type of
	the second argument to bfd_boolean (*) (bfd *, int, char *).
	Return false if FUNC returns false.
	(tekhex_object_p): Return NULL if pass_over fails.


diff -uprN ORIG/src/bfd/tekhex.c LOCAL/src/bfd/tekhex.c
--- ORIG/src/bfd/tekhex.c	2005-05-24 02:44:55.000000000 +0900
+++ LOCAL/src/bfd/tekhex.c	2006-04-22 11:24:05.000000000 +0900
@@ -333,7 +333,7 @@ insert_byte (bfd *abfd, int value, bfd_v
 /* The first pass is to find the names of all the sections, and see
   how big the data is.  */
 
-static void
+static bfd_boolean
 first_phase (bfd *abfd, int type, char *src)
 {
   asection *section = bfd_abs_section_ptr;
@@ -355,9 +355,12 @@ first_phase (bfd *abfd, int type, char *
 	  }
       }
 
-      return;
+      return TRUE;
     case '3':
       /* Symbol record, read the segment.  */
+      if (!ISHEX (*src))
+	return FALSE;
+
       len = getsym (sym, &src);
       section = bfd_get_section_by_name (abfd, sym);
       if (section == NULL)
@@ -400,6 +403,9 @@ first_phase (bfd *abfd, int type, char *
 		abfd->flags |= HAS_SYMS;
 		new->prev = abfd->tdata.tekhex_data->symbols;
 		abfd->tdata.tekhex_data->symbols = new;
+		if (!ISHEX (*src))
+		  return FALSE;
+
 		len = getsym (sym, &src);
 		new->symbol.name = bfd_alloc (abfd, (bfd_size_type) len + 1);
 		if (!new->symbol.name)
@@ -412,16 +418,20 @@ first_phase (bfd *abfd, int type, char *
 		  new->symbol.flags = BSF_LOCAL;
 		new->symbol.value = getvalue (&src) - section->vma;
 	      }
+	    default:
+	      return FALSE;
 	    }
 	}
     }
+
+  return TRUE;
 }
 
 /* Pass over a tekhex, calling one of the above functions on each
    record.  */
 
-static void
-pass_over (bfd *abfd, void (*func) (bfd *, int, char *))
+static bfd_boolean
+pass_over (bfd *abfd, bfd_boolean (*func) (bfd *, int, char *))
 {
   unsigned int chars_on_line;
   bfd_boolean eof = FALSE;
@@ -462,8 +472,11 @@ pass_over (bfd *abfd, void (*func) (bfd 
       /* Put a null at the end.  */
       src[chars_on_line] = 0;
 
-      func (abfd, type, src);
+      if (!func (abfd, type, src))
+	return FALSE;
     }
+
+  return TRUE;
 }
 
 static long
@@ -524,7 +537,9 @@ tekhex_object_p (bfd *abfd)
 
   tekhex_mkobject (abfd);
 
-  pass_over (abfd, first_phase);
+  if (!pass_over (abfd, first_phase))
+    return NULL;
+
   return abfd->xvec;
 }
 

Re: [patch RFC] Fix PR binutils/2584

[Alan Modra]

On Thu, Apr 27, 2006 at 08:06:23AM +0900, Kaz Kojima wrote:
> 	PR 2584
> 	* tekhex.c (first_phase): Change return type to bfd_boolean
> 	and return false if the unexpected character is found.
> 	(pass_over): Change return type to bfd_boolean and the type of
> 	the second argument to bfd_boolean (*) (bfd *, int, char *).
> 	Return false if FUNC returns false.
> 	(tekhex_object_p): Return NULL if pass_over fails.

Thanks, but can I ask that you do this differently?  Add an
unsigned int * param to getsym to return the length, and move the hex
check into getsym.  Return false from getsym on bad hex chars.
Similarly add a bfd_vma * param to getvalue to return the value and
return status via the function return.  Also replace "abort ()" in
first_phase with "return FALSE" while you're at it.  Patch preapproved.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [patch RFC] Fix PR binutils/2584

[Kaz Kojima]

Alan Modra <amodra@bigpond.net.au> wrote:
> Thanks, but can I ask that you do this differently?  Add an
> unsigned int * param to getsym to return the length, and move the hex
> check into getsym.  Return false from getsym on bad hex chars.
> Similarly add a bfd_vma * param to getvalue to return the value and
> return status via the function return.  Also replace "abort ()" in
> first_phase with "return FALSE" while you're at it.  Patch preapproved.

Like this?

Regards,
	kaz
--
	PR 2584
	* tekhex.c (getvalue): Change return type to bfd_boolean and
	add the new parameter.  Return false if the unexpected character
	is found.
	(getsym): Likewise.
	(first_phase): Change return type to bfd_boolean and return
	false if the unexpected character is found.  Replace abort
	with returning false.
	(pass_over): Change return type to bfd_boolean and the type of
	the second argument to bfd_boolean (*) (bfd *, int, char *).
	Return false if FUNC returns false.
	(tekhex_object_p): Return NULL if pass_over fails.

--- ORIG/src/bfd/tekhex.c	2005-05-24 02:44:55.000000000 +0900
+++ LOCAL/src/bfd/tekhex.c	2006-04-27 11:50:17.000000000 +0900
@@ -264,36 +264,46 @@ typedef struct tekhex_data_struct
 
 #define enda(x) (x->vma + x->size)
 
-static bfd_vma
-getvalue (char **srcp)
+static bfd_boolean
+getvalue (char **srcp, bfd_vma *valuep)
 {
   char *src = *srcp;
   bfd_vma value = 0;
-  unsigned int len = hex_value(*src++);
+  unsigned int len;
+
+  if (!ISHEX (*src))
+    return FALSE;
 
+  len = hex_value (*src++);
   if (len == 0)
     len = 16;
   while (len--)
     value = value << 4 | hex_value(*src++);
 
   *srcp = src;
-  return value;
+  *valuep = value;
+  return TRUE;
 }
 
-static unsigned int
-getsym (char *dstp, char **srcp)
+static bfd_boolean
+getsym (char *dstp, char **srcp, unsigned int *lenp)
 {
   char *src = *srcp;
   unsigned int i;
-  unsigned int len = hex_value(*src++);
+  unsigned int len;
+  
+  if (!ISHEX (*src))
+    return FALSE;
 
+  len = hex_value (*src++);
   if (len == 0)
     len = 16;
   for (i = 0; i < len; i++)
     dstp[i] = src[i];
   dstp[i] = 0;
   *srcp = src + i;
-  return len;
+  *lenp = len;
+  return TRUE;
 }
 
 static struct data_struct *
@@ -333,11 +343,12 @@ insert_byte (bfd *abfd, int value, bfd_v
 /* The first pass is to find the names of all the sections, and see
   how big the data is.  */
 
-static void
+static bfd_boolean
 first_phase (bfd *abfd, int type, char *src)
 {
   asection *section = bfd_abs_section_ptr;
   unsigned int len;
+  bfd_vma val;
   char sym[17];			/* A symbol can only be 16chars long.  */
 
   switch (type)
@@ -345,7 +356,10 @@ first_phase (bfd *abfd, int type, char *
     case '6':
       /* Data record - read it and store it.  */
       {
-	bfd_vma addr = getvalue (&src);
+	bfd_vma addr;
+
+	if (!getvalue (&src, &addr))
+	  return FALSE;
 
 	while (*src)
 	  {
@@ -355,17 +369,18 @@ first_phase (bfd *abfd, int type, char *
 	  }
       }
 
-      return;
+      return TRUE;
     case '3':
       /* Symbol record, read the segment.  */
-      len = getsym (sym, &src);
+      if (!getsym (sym, &src, &len))
+	return FALSE;
       section = bfd_get_section_by_name (abfd, sym);
       if (section == NULL)
 	{
 	  char *n = bfd_alloc (abfd, (bfd_size_type) len + 1);
 
 	  if (!n)
-	    abort ();		/* FIXME.  */
+	    return FALSE;
 	  memcpy (n, sym, len + 1);
 	  section = bfd_make_section (abfd, n);
 	}
@@ -375,8 +390,11 @@ first_phase (bfd *abfd, int type, char *
 	    {
 	    case '1':		/* Section range.  */
 	      src++;
-	      section->vma = getvalue (&src);
-	      section->size = getvalue (&src) - section->vma;
+	      if (!getvalue (&src, &section->vma))
+		return FALSE;
+	      if (!getvalue (&src, &val))
+		return FALSE;
+	      section->size = val - section->vma;
 	      section->flags = SEC_HAS_CONTENTS | SEC_LOAD | SEC_ALLOC;
 	      break;
 	    case '0':
@@ -393,35 +411,42 @@ first_phase (bfd *abfd, int type, char *
 		char stype = (*src);
 
 		if (!new)
-		  abort ();	/* FIXME.  */
+		  return FALSE;
 		new->symbol.the_bfd = abfd;
 		src++;
 		abfd->symcount++;
 		abfd->flags |= HAS_SYMS;
 		new->prev = abfd->tdata.tekhex_data->symbols;
 		abfd->tdata.tekhex_data->symbols = new;
-		len = getsym (sym, &src);
+		if (!getsym (sym, &src, &len))
+		  return FALSE;
 		new->symbol.name = bfd_alloc (abfd, (bfd_size_type) len + 1);
 		if (!new->symbol.name)
-		  abort ();	/* FIXME.  */
+		  return FALSE;
 		memcpy ((char *) (new->symbol.name), sym, len + 1);
 		new->symbol.section = section;
 		if (stype <= '4')
 		  new->symbol.flags = (BSF_GLOBAL | BSF_EXPORT);
 		else
 		  new->symbol.flags = BSF_LOCAL;
-		new->symbol.value = getvalue (&src) - section->vma;
+		if (!getvalue (&src, &val))
+		  return FALSE;
+		new->symbol.value = val - section->vma;
 	      }
+	    default:
+	      return FALSE;
 	    }
 	}
     }
+
+  return TRUE;
 }
 
 /* Pass over a tekhex, calling one of the above functions on each
    record.  */
 
-static void
-pass_over (bfd *abfd, void (*func) (bfd *, int, char *))
+static bfd_boolean
+pass_over (bfd *abfd, bfd_boolean (*func) (bfd *, int, char *))
 {
   unsigned int chars_on_line;
   bfd_boolean eof = FALSE;
@@ -462,8 +487,11 @@ pass_over (bfd *abfd, void (*func) (bfd 
       /* Put a null at the end.  */
       src[chars_on_line] = 0;
 
-      func (abfd, type, src);
+      if (!func (abfd, type, src))
+	return FALSE;
     }
+
+  return TRUE;
 }
 
 static long
@@ -524,7 +552,9 @@ tekhex_object_p (bfd *abfd)
 
   tekhex_mkobject (abfd);
 
-  pass_over (abfd, first_phase);
+  if (!pass_over (abfd, first_phase))
+    return NULL;
+
   return abfd->xvec;
 }
 

Re: [patch RFC] Fix PR binutils/2584

[Alan Modra]

On Thu, Apr 27, 2006 at 12:14:23PM +0900, Kaz Kojima wrote:
> -static bfd_vma
> -getvalue (char **srcp)
> +static bfd_boolean
> +getvalue (char **srcp, bfd_vma *valuep)
>  {
>    char *src = *srcp;
>    bfd_vma value = 0;
> -  unsigned int len = hex_value(*src++);
> +  unsigned int len;
> +
> +  if (!ISHEX (*src))
> +    return FALSE;
>  
> +  len = hex_value (*src++);
>    if (len == 0)
>      len = 16;
>    while (len--)
>      value = value << 4 | hex_value(*src++);

You missed checking for a valid hex number here.  Otherwise the patch
looks OK.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [patch RFC] Fix PR binutils/2584

[Kaz Kojima]

Alan Modra <amodra@bigpond.net.au> wrote:
>>    while (len--)
>>      value = value << 4 | hex_value(*src++);
> 
> You missed checking for a valid hex number here.  Otherwise the patch
> looks OK.

Oops.  I'll change that lines to

   while (len--)
     {
       if (!ISHEX (*src))
	 return FALSE;
       value = value << 4 | hex_value (*src++);
     }

and commit the patch after the test.  Thanks for reviewing.

Regards,
	kaz

Re: [patch RFC] Fix PR binutils/2584

[Kaz Kojima]

Hi Daniel,

Can I backport the fix for PR binutils/2584 on mainline

  http://sourceware.org/ml/binutils/2006-04/msg00390.html
  http://sourceware.org/ml/binutils/2006-04/msg00393.html

to the 2.17 branch?

Regards,
	kaz

Re: [patch RFC] Fix PR binutils/2584

[Daniel Jacobowitz]

On Wed, May 03, 2006 at 09:52:24AM +0900, Kaz Kojima wrote:
> Hi Daniel,
> 
> Can I backport the fix for PR binutils/2584 on mainline
> 
>   http://sourceware.org/ml/binutils/2006-04/msg00390.html
>   http://sourceware.org/ml/binutils/2006-04/msg00393.html
> 
> to the 2.17 branch?

Yes - please do!

-- 
Daniel Jacobowitz
CodeSourcery