This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [nosys] ARM heap overflow check in libnosys


On Fri, Feb 28, 2014 at 6:30 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 28/02/14 10:27, Richard Earnshaw wrote:
>> On 27/02/14 04:43, Joey Ye wrote:
>>> Original libnosys doesn't check heap overflow. This patch adds an ARM target
>>> specific check there, without breaking other targets.
>>>
>>> OK to upstream?
>>>
>>>
>>> ChangeLog (libgloss):
>>>
>>> 2014-02-27  Joey Ye  <joey.ye@arm.com>
>>>         libnosys/sbrk.c (errno.h): New include.
>>>         (errno): Redefine.
>>>         (STACK_PTR): Define macro for __arm__.
>>>         (_sbrk): Check STACK_PTR.
>>>
>>
>> I can't approve or reject this, since it's a change to generic code.
>> However, some comments:
>>
>> 1) You assume that the stack grows downwards and the heap upwards.  On
>> some machines the stack can grow upwards.  I'm not sure if machines like
>> this are interesting in this case.
Will check that and do nothing in case of stack growing downwards.

>> 2) You assume that there's a single stack and that the stack is not
>> allocated within the heap (threaded code often does this)
This patch is based on a whole lot of assumptions that won't work in
all cases. But IMHO it is inlined with what libnosys intends to do:
only use it when it is not necessary to re-implement. It should only
work without OS, nor multi-threading. I think it a reasonable
assumption that this _sbrk is only used for single thread.

>> 3) It would be nice if the machine-specific definition could be moved
>> out into a machine-specific header.
Will try it
>> 4) I don't think it's safe to undef errno like that.
I don't like it either but it is the way that unlink, wait and write
uses. Again, it assumes single thread
>>
>
> And one more:
>
> 5) You leave no grace space between the stack and the heap.  Unless you
> are in the innermost frame at this time (deepest stack usage) you'll
> likely later end up with a conflict when the stack moves down into the heap.
This is the case that overflow checking in _sbrk won't work. No matter
how much buffer it leaves, stack may grow later to crash into heap.
But this check is better than nothing.

Thanks,
Joey
>
> R.
>
>> R.
>>
>>> diff --git a/libgloss/libnosys/sbrk.c b/libgloss/libnosys/sbrk.c
>>> index 86c130a..910db20 100644
>>> --- a/libgloss/libnosys/sbrk.c
>>> +++ b/libgloss/libnosys/sbrk.c
>>> @@ -2,6 +2,16 @@
>>>
>>>  #include "config.h"
>>>  #include <_syslist.h>
>>> +#include <errno.h>
>>> +#undef errno
>>> +extern int errno;
>>> +
>>> +#undef STACK_PTR
>>> +#ifdef __arm__
>>> +/* Register name faking - works in collusion with the linker.  */
>>> +register char *stack_ptr asm ("sp");
>>> +#define STACK_PTR stack_ptr
>>> +#endif
>>>
>>>  void *
>>>  _sbrk (incr)
>>> @@ -15,6 +25,15 @@ _sbrk (incr)
>>>       heap_end = & end;
>>>
>>>     prev_heap_end = heap_end;
>>> +
>>> +#ifdef STACK_PTR
>>> +  if (heap_end + incr > STACK_PTR)
>>> +    {
>>> +      errno = ENOMEM;
>>> +      return (void *) -1;
>>> +    }
>>> +#endif
>>> +
>>>     heap_end += incr;
>>>
>>>     return (void *) prev_heap_end;=
>>>
>>>
>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]