Fwd: [patch] bfd/som.c: fix local buffer overrun (was: gas (binutils) 2.10: SIGSEGV on hppa1.1-hp-hpux10.20)

[Marco Franzen <marcof at thyron dot com>]

[Sorry, I mistyped the address on my original crossposting to this list.
The original posting was 
http://sources.redhat.com/ml/bug-gnu-utils/2000-09/msg00048.html 
This is the right list for patches, isn't it?]
----------  Forwarded Message  ----------
Regarding my earlier bug report:
http://sources.redhat.com/ml/bug-gnu-utils/2000-08/msg00228.html

I've found the bug.  It was indeed an unchecked local buffer overrun in the HP
PA-RISC specific bfd backend, which led to a corrupt stack.  Appended is a
patch that fixes it.

I wasn't entirely sure that alloca is always safe if used together with nested
scopes.  (Especially when you do an alloca in a nested scope, leave it and
enter another nested scope (as happens here): When re-using the stack space of
the old scope, whether the automatic space of the new scope (if larger) might
overlap with the alloca-ted area.)  To be on the safe side, I moved (and
collapsed) the two declarations of `length' up to function-scope.  I also
changed the type of `length' from int to size_t, even if that is a bit
pedantic.

Incidentall, isn't that fixed-sized buffer banned by GNU coding standards?

Here's the patch. (BTW, I am not on this list. If you could make sure that
you CC me any conversation about this please? Thanks.)

diff -U5 bfd/som.c.orig bfd/som.c
--- /usr/gnu/src/binutils-2.10/bfd/som.c.orig    Sat Feb 26 01:45:22 2000
+++ /usr/gnu/src/binutils-2.10/bfd/som.c         Fri Sep  8 18:05:00 2000
@@ -1,7 +1,7 @@
/* bfd back-end for HP PA-RISC SOM objects.
-   Copyright (C) 1990, 91, 92, 93, 94, 95, 96, 97, 1998
+   Copyright (C) 1990, 91, 92, 93, 94, 95, 96, 97, 1998, 2000
Free Software Foundation, Inc.

Contributed by the Center for Software Science at the
University of Utah (pa-gdb-bugs@cs.utah.edu).

@@ -2750,10 +2750,13 @@
          the relocation queue. 

          No single BFD relocation could ever translate into more
          than 100 bytes of SOM relocations (20bytes is probably the
          upper limit, but leave lots of space for growth).  */
+#if 100 > SOM_TMP_BUFSIZE
+#error "SOM_TMP_BUFSIZE too small"
+#endif
       if (p - tmp_space + 100 > SOM_TMP_BUFSIZE)
         {
           if (bfd_write ((PTR) tmp_space, p - tmp_space, 1, abfd)
               != p - tmp_space)
             return false;
@@ -3083,10 +3086,14 @@
       != p - tmp_space) 
     return false;
   /* Reset to beginning of the buffer space.  */
   p = tmp_space;
 }
+      /* Double check it fits in the now empty buffer.  If this should
+         ever fail, we will need to fix it using alloca as below.  */
+      if (5 + length > SOM_TMP_BUFSIZE)
+        abort ();

/* First element in a string table entry is the length of the
  string.  Alignment issues are already handled.  */
bfd_put_32 (abfd, length, p);
p += 4;
@@ -3134,11 +3141,13 @@
{
unsigned int i;

/* Chunk of memory that we can use as buffer space, then throw
away.  */
-  unsigned char tmp_space[SOM_TMP_BUFSIZE];
+  size_t tmp_space_size = 64; /* or SOM_TMP_BUFSIZE */
+  unsigned char *tmp_space = alloca (tmp_space_size);
+  size_t length;
unsigned char *p;
unsigned int strings_size = 0;
unsigned char *comp[4];

/* This gets a bit gruesome because of the compilation unit.  The
@@ -3153,11 +3162,17 @@
comp[1] = compilation_unit->language_name.n_name;
comp[2] = compilation_unit->product_id.n_name;
comp[3] = compilation_unit->version_id.n_name;
}

-  memset (tmp_space, 0, SOM_TMP_BUFSIZE);
+#if 0
+  /* This is probably useless, as long as it is done only before the
+     first filling of the buffer.  If there should be any reason to
+     keep this, there will also be reason to do this each time before
+     refilling the buffer, or at least after reallocating the buffer.  */
+  memset (tmp_space, 0, tmp_space_size);
+#endif
p = tmp_space;

/* Seek to the start of the space strings in preparation for writing
them out.  */
if (bfd_seek (abfd, current_offset, SEEK_SET) < 0)
@@ -3165,21 +3180,44 @@

if (compilation_unit)
{
for (i = 0; i < 4; i++)
 {
-          int length = strlen (comp[i]);
+          /* Not a local variable to avoid confusing any
+             implementations of alloca. */
+          length = strlen (comp[i]);

   /* If there is not enough room for the next entry, then dump
-             the current buffer contents now.  */
-          if (p - tmp_space + 5 + length > SOM_TMP_BUFSIZE)
+             the current buffer contents now and maybe allocate a
+             larger buffer.  */
+          if (p - tmp_space + 5 + length > tmp_space_size)
     {
+              /* Flush buffer before refilling or reallocating. */
       if (bfd_write ((PTR) &tmp_space[0], p - tmp_space, 1, abfd)
           != p - tmp_space)
         return false;
-              /* Reset to beginning of the buffer space.  */
+
+              /* Reallocate if now empty buffer still too small.  */
+              if (5 + length > tmp_space_size)
+                {
+                  /* Ensure a minimum growth factor to avoid O(n**2)
+                     space consumption for n strings.  The optimum
+                     minimal factor seems to be 2, as no other value
+                     can guarantee wasting less then 50% space.  (Note
+                     that we cannot deallocate space allocated by
+                     `alloca' without returning from this function.)  */
+                  tmp_space_size = MAX (2 * tmp_space_size, 5 + length);
+                  tmp_space = alloca (tmp_space_size);
+                }
+
+              /* Reset to beginning of the (possibly new) buffer
+                 space.  */
       p = tmp_space;
+#if 0
+              /* See `memset' above for discussion of usefulness.  */
+              memset (tmp_space, 0, tmp_space_size);
+#endif
     }

   /* First element in a string table entry is the length of
      the string.  This must always be 4 byte aligned.  This is
      also an appropriate time to fill in the string index
@@ -3223,21 +3261,42 @@
 }
}

for (i = 0; i < num_syms; i++)
{
-      int length = strlen (syms[i]->name);
+      /* Not a local variable to avoid confusing any implementations
+         of alloca. */
+      length = strlen (syms[i]->name);

/* If there is not enough room for the next entry, then dump the
-         current buffer contents now.  */
-     if (p - tmp_space + 5 + length > SOM_TMP_BUFSIZE)
+         current buffer contents now and maybe allocate a larger buffer.  */
+     if (p - tmp_space + 5 + length > tmp_space_size)
 {
+          /* Flush buffer before refilling or reallocating. */
   if (bfd_write ((PTR) &tmp_space[0], p - tmp_space, 1, abfd)
       != p - tmp_space)
     return false;
-          /* Reset to beginning of the buffer space.  */
+
+          /* Reallocate if now empty buffer still too small.  */
+          if (5 + length > tmp_space_size)
+            {
+              /* Ensure a minimum growth factor to avoid O(n**2) space
+                 consumption for n strings.  The optimum minimal
+                 factor seems to be 2, as no other value can guarantee
+                 wasting less then 50% space.  (Note that we cannot
+                 deallocate space allocated by `alloca' without
+                 returning from this function.)  */
+              tmp_space_size = MAX (2 * tmp_space_size, 5 + length);
+              tmp_space = alloca (tmp_space_size);
+            }
+
+          /* Reset to beginning of the (possibly new) buffer space.  */
   p = tmp_space;
+#if 0
+          /* See `memset' above for discussion of usefulness.  */
+          memset (tmp_space, 0, tmp_space_size);
+#endif
 }

/* First element in a string table entry is the length of the
  string.  This must always be 4 byte aligned.  This is also
  an appropriate time to fill in the string index field in the

-----------------------------------------------------------------
This email is confidential and intended solely for the use of the
individual to whom it is addressed.
Any views or opinions presented are solely those of the author
and do not necessarily represent those of Thyron Limited.
If you are not the intended recipient then please be advised
that you have received this email in error and that any use,
dissemination, forwarding, printing or copying of this email
is strictly prohibited.
If you have received this email in error, please notify the
Thyron IT Administrator on +44 (0)1923 236 050 or
send an email to mail-admin@thyron.com.
Thank You