Overflow check missing in _sbrk_r ?

[Pot8o-s]

I am not sure about all the check that are being done by the newlib's _malloc_r implementation, but in your heap_useNewlib_ST.c file, isn't it safer to also add the following check in your _sbrk_r implementation to make sure we do not overflow the currentHeapEnd pointer:

    ....
    DRN_ENTER_CRITICAL_SECTION(usis);

    if (incr < 0 && currentHeapEnd + incr > currentHeapEnd ||
        incr > 0 && currentHeapEnd + incr < currentHeapEnd) {
        // Fail here because currentHeapEnd will be overflown.
    }

    if (currentHeapEnd + incr > limit) {
        ...

[DRNadler]

Sorry, I'm afraid I don't understand your question? Both provided implementations check for out-of-memory with the check: if (currentHeapEnd + incr > limit) { // Ooops, no more memory available... The limit checked against accounts for heap allocation. Have I missed something? Thanks, Best Regards, Dave

[Pot8o-s]

Imagine currentHeapEnd points to the address 0x80000001, limit equal 0x80001000, and we try to allocate INT_MAX (I believe it does not work, because malloc is checking for INT_MAX specificaly, but let's just imagine), currentHeapEnd + incr will equal NULL (because of the overflow), and NULL is smaller than limit, so, from what I see, your allocation will "succeed", but it should fail.

[DRNadler]

Go it, yes could overflow depending on memory layout. Not an issue for the targets I'm using (because SRAM is not near top of 32-bit address space), but certainly possible. Not sure the best way (arithmetically/code) to ensure no overflow...

[Pot8o-s]

I also don't know exactly how newlib's implementation of free works, but we could also maybe risk an overflow on the other side if the SRAM is located at the bottom of the 32-bit address space if called with INT_MIN.

Anyhow, just wanted to point it out in case. Thank you for your precious thread safe explanation and implementation of _sbrk, really helped.

Best regards

[DRNadler]

Thank you for your precious thread safe explanation and implementation of _sbrk, really helped.

Thanks, much appreciated!

[Pot8o-s]

I've noticed that the pointer comparison I've proposed in my first comment above does not work.

See https://stackoverflow.com/a/6702196/11512417

The following should be correct:

    ....
    DRN_ENTER_CRITICAL_SECTION(usis);

    if (incr < 0 && (uintptr_t)(currentHeapEnd + incr) > (uintptr_t)currentHeapEnd ||
        incr > 0 && (uintptr_t)(currentHeapEnd + incr) < (uintptr_t)currentHeapEnd) {
        // Fail here because currentHeapEnd will be overflown.
    }

    if (currentHeapEnd + incr > limit) {
        ...

In this regards, there might also be an issue in your line if (currentHeapEnd + incr > limit) {. It might need to be changed to if ((uintptr_t)(currentHeapEnd + incr) > (uintptr_t)limit) { to ensure the pointer comparison is done correctly.

[DRNadler]

A pull-request including arithmetic tests would be appreciated, Thanks!