4

I'm finalizing my function for safe string retrieval and decided to turn my compiler warnings up to see if any of my code raised any flags.

Currently I am receiving the following compiler warnings on Pelles C IDE:

stringhandle.c(39): warning #2800: Potentially dangling object 'str' used after call to function 'realloc'.
stringhandle.c(50): warning #2800: Potentially dangling object 'str' used after call to function 'realloc'.

Here is my function (read below if you would rather read the question in it's entirety before reading code):

char *getstr(void)
{
    char *str, *tmp;
    int bff = STRBFF, ch = -1, pt = 0;

    if(!(str = malloc(bff)))
    {
        printf("\nError! Memory allocation failed!");
        return 0x00;
    }
    while(ch)
    {
        ch = getc(stdin);
        if (ch == EOF || ch == '\n' || ch == '\r') ch = 0;
        if (bff <= pt)
        {
            bff += STRBFF; 
            if(!(tmp = realloc(str, bff))) 
            {
                free(str);  //line 39 triggers first warning
                str = 0x00;
                printf("\nError! Memory allocation failed!");
                return 0x00;
            }
            str = tmp;
        }
        str[pt++] = (char)ch;
    }
    str[pt] = 0x00;
    if(!(tmp = realloc(str, pt)))
    {
        free(str); //line 50 triggers second warning
        str = 0x00;
        printf("\nError! Memory allocation failed!");
        return 0x00;
    }
    str = tmp;
    return str;
}

I think understand why I am being warned that str may be dangling. I am freeing the allocated space pointed to by str if an error occurs, however my function has no further calls to str after it being freed. As a fix, I just tried doing free(str) followed by str = 0x00. Shouldn't that make the pointer str no longer dangling? Does it have something to do with my tmp pointer? I don't free or set tmp to 0x00 either, since it should already be 0x00 if realloc fails. But should I be setting it to 0x00 on success, since it is still technically pointing exactly where str is and is no longer needed?

In short:

  1. Why is my compiler warning that str may be dangling?
  2. How can I remove the warning?
  3. Am I handling my tmp pointer correctly?
Palec
  • 12,743
  • 8
  • 69
  • 138
Keith Miller
  • 1,337
  • 1
  • 16
  • 32
  • 1
    Ignore the warning. Get used to false positives. Compilers aren't intelligent. –  Sep 05 '12 at 20:17
  • @stark those lines both refer to `free(str)` respectively – Keith Miller Sep 05 '12 at 20:25
  • 1
    Then your compiler is indeed stupid. `free(str)` is the sensible thing to do. – Fred Foo Sep 05 '12 at 20:26
  • 2
    then there's no reason to have str = 0. H2CO3 is correct. – stark Sep 05 '12 at 20:28
  • 3
    1) you don't need the initial malloc(). Just initialise str =NULL, and the first `realloc(NULL, newsize)` will work as you intend. 2) your final `str[pt] = 0x00;` is not needed (when the loop terminates a nul will already have been written (it is the loop condition!) It is also wrong, because after the loop, pt has been bumped and `(bff <= pt)` could be true. That would cause the final `str[pt] = 0;` to actually write beyond the allocated size. 3) don't return 0x00 when you can return NULL, it is ugly and confusing. 4) most people would write `(pt >= bff)` instead of `(bff <= pt)` – wildplasser Sep 05 '12 at 20:29
  • OH nevermind I see what you're saying now I misread. – Keith Miller Sep 05 '12 at 20:37
  • 1
    `if (ch == EOF || ch == '\n' || ch == '\r') ch = 0;` Indirectly, that is your loop condition. ad 1) your initial size is the same as your chunked increment (both are STRBFF), so it would be the same. – wildplasser Sep 05 '12 at 20:38
  • If I don't allocate in the beginning wouldn't I get a segmentation fault? I would be dereferencing a null pointer, since realloc would never be called (`bff` initially holds a value of `256`). – Keith Miller Sep 05 '12 at 20:44
  • 1
    @Keith: When you post errors that cite specific line numbers, and then a block of code, it's extremely useful if you add a comment to the code identifying the lines in question. It makes it much easier for people to see what lines the issue is in without trying to count them, and you easily have access to that info in most editors. – Ken White Sep 05 '12 at 20:48
  • @Ken noted, thank you very much I will update my post now. – Keith Miller Sep 05 '12 at 20:49
  • 1
    @KeithMiller, you wouldn't need `malloc` at the beginning if you initialized `str` to `NULL` and `bff` to `0`. – eq- Sep 05 '12 at 20:50
  • Ah of course, wow this is really cool, I learned a lot more (obvious things) than I bargained for by posting this question, thank you. – Keith Miller Sep 05 '12 at 20:51

1 Answers1

1

Just to illustrate my points:

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

static inline void * myrealloc(void *org, size_t newsize)
{
char * new;
new = realloc(org, newsize);
if (!new) {
        fprintf(stderr, "\nError! Memory allocation failed!\n");
        free (org);
        return NULL;
        }
return new;
}

char *getstr(void)
{
#define STRBFF 256

    char *str = NULL;
    size_t size , used ;

    for (size=used=0; ; ) {
        int ch;
        if (used >=size) {
            str = myrealloc(str, size += STRBFF);
            if(!str) return NULL;
        }
        ch = getc(stdin);
        if (ch == EOF || ch == '\n' || ch == '\r') ch = 0;
        str[used++] = ch;
        if (!ch) break;
    }
    str = myrealloc(str, used);
    return str;
}

Now, if your compiler supports inlining, the calls to myrealloc() will be replaced by the equivalent of your original code, and the actual myrealloc()function will virtually disappear. (check the disassembled output).

wildplasser
  • 43,142
  • 8
  • 66
  • 109