This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [m68k] Final part of architecture cleanup
- From: Nathan Sidwell <nathan at codesourcery dot com>
- To: Ben Elliston <bje at air dot net dot au>
- Cc: binutils at sourceware dot org
- Date: Fri, 24 Mar 2006 07:40:36 +0000
- Subject: Re: [m68k] Final part of architecture cleanup
- References: <20060312230407.GA32449@ozlabs.au.ibm.com> <20060324160530.A32499@mailhub.air.net.au>
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");