Problem with realloc() in C. Always hangs but compiles fine...

Question

Hi there, I'm having some trouble with a program that is intended to be a String buffer, specifically this function is intended to reset the buffer with the string cstr. If cstr is null then the content needs to be reset to an empty char '\0'. It always hangs at the second set of realloc where it's resizing buf->contents I have no clue why that is. Any help would be awesome.

The struct:

typedef struct strbuf {
     char   *contents;
     size_t  length;  
} StringBuffer;

It is called from

strbuf_reset(sb, NULL)

Here is the strbuf_reset function that is having the problem.

StringBuffer *strbuf_reset(StringBuffer *buf, const char *cstr)
{
if(buf == NULL)
    return NULL;

StringBuffer *tempBuf = NULL ;

if(cstr == NULL)
    tempBuf = (StringBuffer*)realloc(buf,sizeof(StringBuffer) + sizeof(char));
else
    tempBuf = (StringBuffer*)realloc(buf,sizeof(buf) + strlen(cstr)*sizeof(char));

if(tempBuf == NULL)
    return NULL;

if(cstr == NULL)
    tempBuf->contents = (char*)realloc(buf->contents,sizeof(char));
else
    tempBuf->contents = (char*)realloc(buf->contents,(sizeof(buf->contents) + strlen(cstr)*sizeof(char) + 1));

if(tempBuf->contents == NULL){
    free(tempBuf);
    return NULL;
}
buf = tempBuf;

if(cstr == NULL)
   buf->contents = '\0';
else
   strcat(buf->contents,cstr);

buf->length = strlen(buf->contents);    

return buf;
 }

With what I believe to be the suggested changes...

StringBuffer *strbuf_reset(StringBuffer *buf, const char *cstr)
{
if(buf == NULL)
    return NULL;

StringBuffer *tempBuf = NULL ;

if(cstr == NULL)
    tempBuf = (StringBuffer*)realloc(buf,sizeof(StringBuffer) + sizeof(char) + 10);
else
    tempBuf = (StringBuffer*)realloc(buf,sizeof(buf) + strlen(cstr)*sizeof(char)+ 1);

if(tempBuf != NULL)
    buf = tempBuf;
else
    return NULL;    

if(cstr == NULL)
    tempBuf->contents = (StringBuffer*)realloc(buf->contents,sizeof(StringBuffer) + sizeof(char) + 10);
else
    tempBuf->contents = (StringBuffer*)realloc(buf->contents,sizeof(buf) + strlen(cstr)*sizeof(char)+ 1);

if(tempBuf != NULL)
    buf->contents = tempBuf->contents;
else
    return NULL;

if(cstr == NULL)
   buf->contents = '\0';
else
   strcat(buf->contents,cstr);

buf->length = strlen(buf->contents);    

return buf;
 }

Answer 1

You don't seem to understand what realloc does.

The way you should be thinking about it (at least as far as enlarging goes), is that it allocates a new buffer, copies the old data into it, and then frees the old buffer.

The old pointer is then invalid and it should not be suprising if you get crashes when you try and use it again later.

You should be assigning the returned value back to the old pointer immediately, so it still points at valid data.

Answer 2

You are allocating extra space for the string in the StringBuffer. I just assumed buf->contents is supposed to point into that extra allocated space. If this is not true then why is the extra space allocated in the StringBuffer?

If buf->contents already points into memory that was allocated for StringBuffer, attempting to realloc it will throw the memory system into a crash/hang/corrupt heap situation, because you would be realloc'ing a pointer that was never allocated.

Instead of attempting to realloc buf->contents, I think the structure should look like:

struct StringBuffer {
    size_t length;
    char contents[1];
};

Then instead of reallocing buf->contents you just copy the string into there and the realloc for StringBuffer takes care of all the memory.

Answer 3

Since you are going to overwrite StringBuffer's contents, using realloc makes no sense. It will not save you any memory allocation but instead will copy old data you intent to overwrite anyway. Use normal malloc and free.

With your original structure

typedef struct strbuf {
  char   *contents;
  size_t  length;  
} StringBuffer;

strbuf_reset sets buf to cstr. On success returns buf, on failure NULL.

StringBuffer *strbuf_reset(StringBuffer *buf, const char *cstr)
{
  if (!buf)  return NULL;      
  if (!cstr)  cstr = "";

  size_t len = strlen(cstr);
  if (len > buf->length) {
    char *new_contents = malloc(len + 1);
    if (!new_contents)  return NULL;
    free(buf->contents);
    buf->contents = new_contents;
  }
  memcpy(buf->contents, cstr, len + 1);
  buf->length = len;

  return buf;
}

Answer 4

Time to use malloc and free instead of all this realloc business. Everything is being overwritten anyway. Thanks everybody. Deffinitely better with realloc anyway now!