realloc incorrectly frees the old block in the case where malloc fails to allocate a larger block

[apmorton]

As per ansi spec, realloc is supposed to return null and leave the original pointer in tact in the case of failure if size > 0 and the original pointer was not null.

The code in this repo violates this condition in the OOM case by calling free on the original pointer regardless of whether the call to malloc suceeds

[rhempel]

Thanks for pointing this out! I will look into this one in the next days.

[d-a-v]

umm_malloc is currently heavily used in esp8266/Arduino.
Here is a simple arduino sketch that trigger this realloc() bug.

To trigger the bug, put this realloc.c file below into your test directory, and run the commented shell command:
without free() it works (it shouldn't), with free() it segfaults.

// mkdir -p dbglog; touch dbglog/dbglog.h; gcc -I. -DDBGLOG_TRACE=printf -DDBGLOG_ERROR=printf -DDBGLOG_DEBUG=printf -D"DBGLOG_FORCE(a,b...)=printf(b)" -DUMM_DBG_LOG_LEVEL=0 -Wall -Wextra realloc.c ../src/*.c -o realloc && ./realloc

#include <stdio.h>
#include <stdint.h>

#include "../src/umm_malloc_cfg.h"
#include "../src/umm_malloc.h"

#define os_printf printf
#define malloc umm_malloc
#define free umm_free
#define realloc umm_realloc

char test_umm_heap [UMM_MALLOC_CFG_HEAP_SIZE];

#define INC 100
size_t len = 0;
char* buf = NULL;

void loop()
{
  if (!buf)
  {
    os_printf(":1\n");
    buf = (char*)malloc(INC);
    if (buf)
      len = INC;
    else
      os_printf(":1null\n");
  }
  else
  {
    os_printf(":re %d -> %d\n", (int)len, (int)len + INC);
    char* newbuf = (char*)realloc(buf, len + INC);
    if (!newbuf)
    {
      os_printf(":re null\n");
      free(buf); //  <--- BUF IS NO MORE ALLOCATED WHERE IT SHOULD BE
      len = 0;
      buf = NULL;
    }
    else
    {
      len += INC;
      buf = newbuf;
    }
  }
}


int main (void)
{
	while (1)
		loop();
}

[kwagyeman]

This isn't really a bug per say. It's just how it works. You're going to have to change a lot of the code in the library to get around this issue.

I worked around it by just accepting that I might get a null pointer back and handling that.

[apmorton]

Kwabena, This is most certainly a bug. the c specification is very clear about the correct behavior for realloc in the case of memory allocation failure the original pointer should still be valid, but umm_malloc is freeing the original pointer when memory allocation fails. The bug has nothing to do with realloc returning a null pointer, that is expected behavior if there is not enough memory for the requested allocation. The issue is that it also invalidates the original pointer you gave to realloc when returning null. Austin On Dec 12, 2017 8:31 PM, "Kwabena W. Agyeman" <notifications@github.com> wrote: This isn't really a bug per say. It's just how it works. You're going to have to change a lot of the code in the library to get around this issue. I worked around it by just accepting that I might get a null pointer back and handling that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD4lMs4ekFQooPMXUirmZtAuxrjgVJEks5s_yjxgaJpZM4QdFGn> .

[kwagyeman]

Sorry, I wasn't getting at that it's not following the C spec.

Yes, of course the behavior right now is not like the correct realloc.

[rhempel]

I believe that I have a solution for this issue. I have also added test cases for realloc() that prove that the assimilate up, down, and up/down as well as new blocks that fit and new blocks that do not fit work correctly. Well, at least for the test cases that I wrote :-)

It was fun to write the test case for the failure described in the issue, watch it fail, and then write code that fixed the problem. And then it was even more fun to realize all the cases being hanlded in realloc and writing test cases for thaem as well.

Please check out the issue-11 branch to see if this addresses the issue for you, and let me know if there is a significant performance change.

[rhempel]

Hang on @d-a-v :-) I have improved the code a bit since yesterday - please review and let me know if this still passes your tests. I have cleaned up the different cases for reallocating using blocks around the current block and I believe that it's much easier to read and understand now.

If it's good enough for you, then I will merge back to the master branch and make a new release, OK?

[d-a-v]

Thanks for working on this.
I'll have a look to your self tests as soon as possible.
Anyway feel free to merge since the original issue is still present in master.

[rhempel]

Thanks for the vote of confidence :-) I like to have an independent verification that the bug is fixed, and no new issues have been introduced. Even if you do not use the Unity (tdd tool, not GUI tool) test suite, please verify that the esp8266/Arduino usage is working as expected now.

By the way, thanks for the support of the Arduino community in picking up umm_malloc - it's really inspiring to see the little library finally finding a good niche.

[grojguy]

I am using this stellar library heavily on several Microchip based projects (PIC24 and PIC32) and have been quite impressed! On the PIC32 project with Harmony, I replaced the stdlib malloc used by the TCP/IP stack. So, umm_malloc is definitely getting a serious workout!

However, I was bit by this bug! But thankfully, the fix solved it for me. Nice job!

[rhempel]

Based on this independent confirmation that the issue-11 branch fixes the problem, I have merged the changes into the master branch and will close this issue.