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

[Jeffrey A Law <law at cygnus dot com>]

  In message <00091215194600.00498@Maidavale>you write:
  > On Tue, 12 Sep 2000, Jeffrey A Law wrote:
  > > You should explain in detail why the buffer overran.
  > 
  > Because it was of hardwired size and too small for the input file.
  > 
  > +              /* Reallocate if now empty buffer still too small.  */
  > +              if (5 + length > tmp_space_size)
  > +                {
  > [...]
  > +                  tmp_space_size = MAX (2 * tmp_space_size, 5 + length);
  > +                  tmp_space = alloca (tmp_space_size);
  > +                }
  > 
  > Gas crashed because the buffer tmp_space in som_write_symbol_strings() was
  > smaller (SOM_TMP_BUFSIZE, 8192) than a single symbol (9172) in the input
  > file of my original bug report.  You can also reproduce it with this
  > single-line input file containg just that symbol (as a label).
Thank you.  That's precisely what I needed to know (you had a symbol with a
very long name).

I just checked the SOM reference and the limit on symbol names is 2^32
characters; so the name is valid according to the SOM reference.

[ ... ]
  > Since no fixed value of SOM_TMP_BUFSIZE fits all valid input data, I fixed
  > it by using alloca.
Seems reasonable.  I was pretty sure that's what the problem was, but as I
mentioned before, it's far better for you to explicitly state the problem
you ran into rather than hoping that my guess is correct! :-)

  > Doing so, I also looked at the other places that use the same fixed size
  > (for different purposes!) and either applied the same fix or put in
  > assertions that we don't need it. 
We do need the fix applied to the code which handles space/subspace
strings.  We do not need the fix applied to the code which handles
relocations/fixups (no single fixup can be 8kbytes).

  > At the same time, I also peephole-reviewed the code I was changing and
  > inserted comments where I found things strange. I meant that either for
  > people with a deeper understanding to replace my suspicions with comments
  > explaining why it is correct. Or to help future maintainers finding these
  > places in case the strangenesses do constitute bugs.
I don't have a problem with the insertion of comments :-)  I know this
code fairly well, but things that make it clearer to you are also likely
going to make it clearer to others that may try to understand the code
in the future.

  > >   > I wasn't entirely sure that alloca is always safe if used together wi
  > th
  > >   > nested scopes. 
  > > It is.  
  > > 
  > >   > (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 la
  > rger)
  > >   > might overlap with the alloca-ted area.)
  > > The alloca'd memory is not released until the current function exits
  > > (or possibly even later).  Thus the situation you mention can not cause
  > > problems.
  > 
  > OK, if you say it is safe by definition of alloca. Basically, I was trying
  > to make 100% sure I am not introducing a new bug. Detecting broken
  > alloca's is a different issue then.
alloca's semantics are such that this is safe (otherwise alloca would be
totally useless).  Any space you allocate with alloca will hang around until
the current function exits (or possibly later).


  > >   > Incidentall, isn't that fixed-sized buffer banned by GNU coding stand
  > ards?
  > > Somewhat.  Depends on the precise situation.  I'll be able to evaluate th
  > e
  > > problem when you provide the analysis mentioned above :-)
  > 
  > Here, the fixed size buffer artificially restricts the set of valid input
  > files the utility can process, and that limit is not even documented to
  > the user (which was g++ in this case;-).
Yes, it is banned in this case.


  > > This statement is completely useless anyway since there are no provisions
  > > for overriding the definition of SOM_TMP_BUFSIZE (which is 8k).
  > 
  > I only meant it to catch situations when someone experimenting with
  > SOM_TMP_BUFSIZE chooses too small a value.
Someone experimenting with that code should take the time to read the
code which uses it.


  > >   > +      /* Double check it fits in the now empty buffer.  If this shou
  > ld
  > >   > +         ever fail, we will need to fix it using alloca as below.  *
  > /
  > >   > +      if (5 + length > SOM_TMP_BUFSIZE)
  > >   > +	abort ();
  > > Technically it is never OK to call abort from within BFD.  Technically we
  > > should return an error and let the application decide what to do.
  > > Unfortunately, som.c was written before that design decision was made and
  > > thus has some scattered aborts.  I'd prefer not to add new ones though by
  > > having new code return errors in the proper way.
  > 
  > Doesn't that depend on whether the error is something like "file system
  > full" or whether it is a bug in the code itself?
No, it doesn't depend on anything like that.

  > I believe the original author meant this could not happen (or they neglecte
  > to think about this condition). Without this assertion, the code would, if
  > it did happen, crash in the strcpy following it, but only after corrupting
  > the stack so that debugging becomes difficult.
I'm the original author.

Basically for the space & symbol strings we should be using alloca'd buffers
and abolish SOM_TMP_BUFSIZE in that code.  That eliminates the need for the
abort/assert completely.

  > But maybe you can rule out anyway that this might ever happen. I don't
  > fully understand what is going on here. Can space names (what are those?)
  > become arbitrarily long because they directly correspond to names in the
  > input file?
Yes.  See above.

  > >   > -  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 befor
  > e
  > >   > +     refilling the buffer, or at least after reallocating the buffer
  > .  */
  > >   > +  memset (tmp_space, 0, tmp_space_size);
  > >   > +#endif
  > > Err, no it is not useless.  There are fields within the various structure
  > s
  > > that we do not use/need, but which must not have garbage values.  The
  > > buffer needs to be cleared.  
  > 
  > My argument was that it is either redundant here or erroneously missing in
  > two other places, namely where the buffer is written out and reused without
  > zeroing it again.
Actually, for the symbol and space/subspace strings it is not needed since any
gaps in them are explicitly zero filled.  Sorry about the confusion.  It's
been about 7 years since I had to look at that code.

You should just delete the zero-ing of the buffers for space/subspace and
symbol strings.

  > Yet another issue, which you will have noticed when replying. The address
  > <pa-gdb-bugs@cs.utah.edu> (mentioned in the header of som.c) is no longer
  > working. Maybe you have contacts there and can confirm that. If the
  > failures are permanent, the address should be either removed or accompanied
  > by a request to send bug reports rather to bug-gnu-utils@gnu.org. (I can
  > include that in my eventual patch.)
The UofU no longer is involved in PA compiler tools work (and hasn't been
since roughly 1995).  Feel free to submit a separate patch to just remove
that address from the header of som.c.

Jeff