Re: [m68k] Final part of architecture cleanup

[Nathan Sidwell <nathan at codesourcery dot com>]

Ben Elliston wrote:
> Hi Nathan

> The problem is that you detect the buffer overflow after it has
> occurred; it will likely be too late.  I'd like to see this code made
> more robust.
>
> How about adding some assertions, at the very least (as the code only
> processes compile-time strings and not external untrusted data)?

Actually I have a follow up patch that fixes the strncpy issue correctly -- I 
found it was still lurking in m68k_ip, and I got bit by it.  I attach that patch.

The attached patch folds find_cf_chip into its only caller and uses it for both 
cf and m68k chips.  Rather than generate the message into a stack allocated 
array, and them copy it to a malloced area, I simply malloc the area first and 
construct directly into it.  Although this might mean the malloced area is 
longer than necessary, I don't really see the point of optimizing that in this 
error handling case.  I use a local macro to do the string appending in a safe 
manner, and you'll see I place a sentinel NUL char right at the end of the 
buffer to start with, so that when strncpy runs out of room, it'll be right 
there.  Then I detect if the buffer is full and replace the last few chars with 
'  ...', rather than die horribly.  (hm, a thought occurs, we could retry with a 
longer buffer -- I'd really like to do that as a separate change though.)

Would you prefer

a) Commit the arch cleanup as is, and then commit the patch I attach here?
b) post a combined patch for this change and the later change.

I would have preferred not fiddling with find_cf_chip at all in this first 
patch, but it needed changing because of how it traversed the cpus array.  I 
found its logic rather tortuous.  I suppose it would be possible to fix the 
overflow in find_cf_chip first, but right now that just seems like make-work.

nathan

--
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

2006-03-21  Nathan Sidwell  <nathan@codesourcery.com>

	* gas/config/tc-m68k.c (find_cf_chip): Merge into ...
	(m68k_ip): ... here.  Use for all chips.  Protect against buffer
	overrun and avoid excessive copying.

Index: gas/config/tc-m68k.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-m68k.c,v
retrieving revision 1.72.2.2
diff -c -3 -p -r1.72.2.2 tc-m68k.c
*** gas/config/tc-m68k.c	21 Mar 2006 10:20:17 -0000	1.72.2.2
--- gas/config/tc-m68k.c	21 Mar 2006 10:23:48 -0000
*************** static char alt_notend_table[256];
*** 802,870 ****
        || (*s == ':'						\
  	  && alt_notend_table[(unsigned char) s[1]])))
  
- /* Return a human readable string holding the list of chips that are
-    valid for a particular architecture, suppressing aliases (unless
-    there is only one of them).  */
- 
- static char *
- find_cf_chip (int architecture)
- {
-   static char buf[1024];
-   char *cp;
-   const struct m68k_cpu *cpu;
-   int any = 0;
-   
-   strcpy (buf, " (");
-   cp = buf + strlen (buf);
- 
-   for (cpu = m68k_cpus; cpu->name; cpu++)
-     if (!cpu->alias && (cpu->arch & architecture))
-       {
- 	const struct m68k_cpu *alias;
- 	if (any)
- 	  {
- 	    strcpy (cp, ", ");
- 	    cp += 2;
- 	  }
- 	any = 0;
- 	strcpy (cp, cpu->name);
- 	cp += strlen (cp);
- 	strcpy (cp, " [");
- 	cp += 2;
- 	if (cpu != m68k_cpus)
- 	  for (alias = cpu - 1; alias->alias; alias--)
- 	    {	
- 	      if (any)
- 		{
- 		  strcpy (cp, ", ");
- 		  cp += 2;
- 		}
- 	      strcpy (cp, alias->name);
- 	      cp += strlen (cp);
- 	      any = 1;
- 	    }
- 	for (alias = cpu + 1; alias->alias; alias++)
- 	  {
- 	    if (any)
- 	      {
- 		strcpy (cp, ", ");
- 		cp += 2;
- 	      }
- 	    strcpy (cp, alias->name);
- 	    cp += strlen (cp);
- 	    any = 1;
- 	  }
- 	
- 	strcpy (cp, "]");
- 	any = 1;
- 	if ((unsigned)(cp - buf) >= sizeof (buf))
- 	  as_fatal (_("coldfire string overflow"));
-       }
-   strcat (cp, ")");
- 
-   return buf;
- }
- 
  #ifdef OBJ_ELF
  
  /* Return zero if the reference to SYMBOL from within the same segment may
--- 802,807 ----
*************** m68k_ip (char *instring)
*** 2067,2158 ****
  	  if (ok_arch
  	      && !(ok_arch & current_architecture))
  	    {
! 	      char buf[200], *cp;
  
! 	      strncpy (buf,
! 		       _("invalid instruction for this architecture; needs "),
! 		       sizeof (buf));
! 	      cp = buf + strlen (buf);
  	      switch (ok_arch)
  		{
  		case mcfisa_a:
! 		  strncpy (cp, _("ColdFire ISA_A"),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
! 		  strncpy (cp, find_cf_chip (ok_arch),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
  		  break;
  		case mcfhwdiv:
! 		  strncpy (cp, _("ColdFire hardware divide"),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
! 		  strncpy (cp, find_cf_chip (ok_arch),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
  		  break;
  		case mcfisa_aa:
! 		  strncpy (cp, _("ColdFire ISA_A+"),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
! 		  strncpy (cp, find_cf_chip (ok_arch),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
  		  break;
  		case mcfisa_b:
! 		  strncpy (cp, _("ColdFire ISA_B"),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
! 		  strncpy (cp, find_cf_chip (ok_arch),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
  		  break;
  		case cfloat:
! 		  strncpy (cp, _("ColdFire fpu"), sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
! 		  strncpy (cp, find_cf_chip (ok_arch),
! 			   sizeof (buf) - (cp - buf));
! 		  cp += strlen (cp);
  		  break;
  		case mfloat:
! 		  strcpy (cp, _("fpu (68040, 68060 or 68881/68882)"));
  		  break;
  		case mmmu:
! 		  strcpy (cp, _("mmu (68030 or 68851)"));
  		  break;
  		case m68020up:
! 		  strcpy (cp, _("68020 or higher"));
  		  break;
  		case m68000up:
! 		  strcpy (cp, _("68000 or higher"));
  		  break;
  		case m68010up:
! 		  strcpy (cp, _("68010 or higher"));
  		  break;
  		default:
  		  {
! 		    int got_one = 0, idx;
  
! 		    for (idx = 0; m68k_cpus[idx].name; idx++)
  		      {
! 			if ((m68k_cpus[idx].arch & ok_arch)
! 			    && ! m68k_cpus[idx].alias)
! 			  {
! 			    if (got_one)
! 			      {
! 				strcpy (cp, " or ");
! 				cp += strlen (cp);
! 			      }
! 			    got_one = 1;
! 			    strcpy (cp, m68k_cpus[idx].name);
! 			    cp += strlen (cp);
! 			  }
  		      }
  		  }
  		}
- 	      cp = xmalloc (strlen (buf) + 1);
- 	      strcpy (cp, buf);
- 	      the_ins.error = cp;
  	    }
  	  else
  	    the_ins.error = _("operands mismatch");
--- 2004,2103 ----
  	  if (ok_arch
  	      && !(ok_arch & current_architecture))
  	    {
! 	      const struct m68k_cpu *cpu;
! 	      int any = 0;
! 	      size_t space = 400;
! 	      char *buf = xmalloc (space + 1);
! 	      size_t len;
! 	      int paren = 1;
! 
! 	      the_ins.error = buf;
! 	      /* Make sure there's a NUL at the end of the buffer -- strncpy
! 		 won't write one when it runs out of buffer */
! 	      buf[space] = 0;
! #define APPEND(STRING) \
!   (strncpy (buf, STRING, space), len = strlen (buf), buf += len, space -= len)
  
! 	      APPEND (_("invalid instruction for this architecture; needs "));
  	      switch (ok_arch)
  		{
  		case mcfisa_a:
! 		  APPEND (_("ColdFire ISA_A"));
  		  break;
  		case mcfhwdiv:
! 		  APPEND (_("ColdFire hardware divide"));
  		  break;
  		case mcfisa_aa:
! 		  APPEND (_("ColdFire ISA_A+"));
  		  break;
  		case mcfisa_b:
! 		  APPEND (_("ColdFire ISA_B"));
  		  break;
  		case cfloat:
! 		  APPEND (_("ColdFire fpu"));
  		  break;
  		case mfloat:
! 		  APPEND (_("M68K fpu"));
  		  break;
  		case mmmu:
! 		  APPEND (_("M68K mmu"));
  		  break;
  		case m68020up:
! 		  APPEND (_("68020 or higher"));
  		  break;
  		case m68000up:
! 		  APPEND (_("68000 or higher"));
  		  break;
  		case m68010up:
! 		  APPEND (_("68010 or higher"));
  		  break;
  		default:
+ 		  paren = 0;
+ 		}
+ 	      if (paren)
+ 		APPEND (" (");
+ 
+ 	      for (cpu = m68k_cpus; cpu->name; cpu++)
+ 		if (!cpu->alias && (cpu->arch & ok_arch))
  		  {
! 		    const struct m68k_cpu *alias;
  
! 		    if (any)
! 		      APPEND (", ");
! 		    any = 0;
! 		    APPEND (cpu->name);
! 		    APPEND (" [");
! 		    if (cpu != m68k_cpus)
! 		      for (alias = cpu - 1; alias->alias; alias--)
! 			{
! 			  if (any)
! 			    APPEND (", ");
! 			  APPEND (alias->name);
! 			  any = 1;
! 			}
! 		    for (alias = cpu + 1; alias->alias; alias++)
  		      {
! 			if (any)
! 			  APPEND (", ");
! 			APPEND (alias->name);
! 			any = 1;
  		      }
+ 		    
+ 		    APPEND ("]");
+ 		    any = 1;
  		  }
+ 	      if (paren)
+ 		APPEND (")");
+ #undef APPEND
+ 	      if (!space)
+ 		{
+ 		  /* we ran out of space, so replace the end of the list
+ 		     with ellipsis.  */
+ 		  buf -= 4;
+ 		  while (*buf != ' ')
+ 		    buf--;
+ 		  strcpy (buf, " ...");
  		}
  	    }
  	  else
  	    the_ins.error = _("operands mismatch");