possible buffer overflow in calls.c?

[Jaka Močnik <jaka at xlab dot si>]

hello!

as this is my first time adding some noise to this list, let me briefly
introduce myself: Jaka; member of a team porting gcc to the NEC SX
architecture ( http://code.google.com/p/sx-gcc/ ).

really close now to a working C compiler, only a few testcases still
failing.

however, I have a problem with the ported SX gcc on a 64-bit host
(x86_64); it crashes due to memory corruption in glibc
( http://code.google.com/p/sx-gcc/issues/detail?id=107 ).

now, imho, the crash can be attributed to a buffer overflow that occurs
constantly, but only manifests itself in a crash when the circumstances
are just right, in calls.c:emit_library_call_value_1(). let me
explain ...

in emit_library_call_value_1(), local var stack_usage_map_buf is
allocated with

  stack_usage_map_buf = XNEWVEC (char, highest_outgoing_arg_in_use);
  stack_usage_map = stack_usage_map_buf;

allocating a buffer with 1 char for each byte of stack required for
outgoing args.

then in a loop iterating over args, upper and lower bounds for indexing
a part of that buffer are calculated with:

#ifdef ARGS_GROW_DOWNWARD
    /* stack_slot is negative, but we want to index stack_usage_map
       with positive values.  */
    upper_bound = -argvec[argnum].locate.offset.constant + 1;
    lower_bound = upper_bound - argvec[argnum].locate.size.constant;
#else
    lower_bound = argvec[argnum].locate.offset.constant;
    upper_bound = lower_bound + argvec[argnum].locate.size.constant;
#endif

the problem with this code is that argvec[argnum].locate.offset is the
offset to the actual arg on the stack *excluding* any padding, while
argvec[argnum].locate.size contains the size *including* any possible
padding (according to comments on struct locate_and_pad_arg_data in
expr.h). therefore, upper_bound for the last arg can be up to a stack
slot size too large than the space allocated for stack_usage_map.

thus a later loop, marking the stack space

  if (ACCUMULATE_OUTGOING_ARGS)
    for (i = lower_bound; i < upper_bound; i++)
      stack_usage_map[i] = 1;

writes past the end of the buffer.

observing this writes with a few printouts confirms that this happens
regularly, on 32- and 64-bit hosts, but most of the time does not cause
problems (the testcase I mention is the only one that crashes the
compiler, and even that one only on 64-bit hosts, while printouts
clearly show that writes past buffer end occur regularly when calling
emit_library_call_value_1()). 

now, the above bounds calculation should imho use slot_offset instead of
offset, which would account for the padding:

#ifdef ARGS_GROW_DOWNWARD
    /* stack_slot is negative, but we want to index stack_usage_map
       with positive values.  */
    upper_bound = -argvec[argnum].locate.slot_offset.constant + 1;
    lower_bound = upper_bound - argvec[argnum].locate.size.constant;
#else
    lower_bound = argvec[argnum].locate.slot_offset.constant;
    upper_bound = lower_bound + argvec[argnum].locate.size.constant;
#endif

so, the above explanation seems ok and the proposed fix seems ok for our
port. however, I'm a bit puzzled at how this could have gone unnoticed,
and not causing any major problems at all for so long. I am also a bit
reluctant to claim that gcc has had this bug in its sources for ...
well ... years ...

now, could someone in the know comment on this, and either give me a
positive nod stating that this is indeed a bug that needs fixing, or
tell me that I don't really have a clue and explain what is wrong with
my above reasoning and what could it be that I am doing wrong ...

btw, base for our port are gcc 4.2.2 sources, however 4.2.4, 4.3.3 and
current trunk sources contain exactly the same code that is described
above.

thanks in advance & best regards,
  jaKa

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/