CODEWITHSUNDEEP
crealloc
Korgan Rivera
Korgan Rivera

Reputation: 495

Where's my pointer error?

I've walked through the following code, but I can't find what's wrong. The function getsxnremem() gets a string up to len chars using fgets(), overwrites the newline (if there is one) with a null-terminator, then re-sizes the memory to fit the string. That's the idea anyway.

The following code sometimes works and sometimes crashes. I've had this happen plenty of times in the past and I usually find the problem, but this time it's taking me too long.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

unsigned getsxnremem(char **str, unsigned len){
    unsigned l, flag = 1;
    free(*str);
    char *buff;
    if ((*str = malloc(len)) == NULL) return 0;
    if(fgets(*str, len, stdin) == NULL) { free(*str); return 0; }
    l = strlen(*str);
    if (l && ((*str)[l-1] == '\n')) { *(str)[l-1] = '\0'; flag = 0; }
    if ((buff = realloc(*str, l + flag)) == NULL){ free(*str); return 0; }
    *str = buff;
    return (l - 1);
}


int main(void){
    char *buff = NULL;
    unsigned l = getsxnremem(&buff, 256);
    printf("%s\n%u chars long.", buff, l);
}

Upvotes: 2

Views: 113

Answers (2)

Sourav Ghosh
Sourav Ghosh

Reputation: 134396

The problem is, you failed to collect the return value of realloc() there.

As per the C11 standard, chapter §7.22.3.5

#include <stdlib.h>
void *realloc(void *ptr, size_t size);

The realloc function deallocates the old object pointed to by ptr and returns a pointer to a new object that has the size specified by size. [...]

realloc() resizes the memory and returns a pointer to the new memory. The old memory is to be free()d, considering realloc() is successful.

So,

  1. You need to collect and check the return value of realloc() and test it against NULL to ensure success. Then, reassign it to *str.

    NOTE: Please do not use a form like p = realloc(p, newsize); because, then, if realloc() fails, you'll end up losing the actual pointer, too.

  2. If realloc() is successful, you must not free() the old pointer. Calling free() on already free()-d memory invokes undefined behavior.

After that, as rightly mentioned in the other answer by dbush, the usage

 { *(str)[l-1] = '\0'; flag = 0; }

is also wrong. Your required string is represented by *str, not str. As per the operator precedence, The Array subscripting operator ([]) has higher precedence over the dereference (*) operator, so essentially your code looks like

{ * ((str)[l-1]) = '\0'; flag = 0; }

Which is not what you want. So, to honor the operator precedence, you should modify it like

{ (*str)[l-1] = '\0'; flag = 0; }

That said, you should also check for the return value of fgets() to ensure the success before you make use of the destination buffer. As malloc() returns unitialized memory, and in case fgets() fails, you'll end up reading from unitialized memory which will again cause UB.

Upvotes: 9

dbush
dbush

Reputation: 225007

For your most recent update, you've got your parenthesis in the wrong place.

This:

if (l && ((*str)[l-1] == '\n')) { *(str)[l-1] = '\0'; flag = 0; }

Should be:

if (l && ((*str)[l-1] == '\n')) { (*str)[l-1] = '\0'; flag = 0; }
                                   ^---- here

Upvotes: 1

Related Questions