0

What I'm working on right now is a state-based parser for any input from a stream. My professor tells me this is the best way to avoid special cases. The way I've got it set up is using functions, and I'm having a little bit of trouble trying to reuse allocated memory so I don't cause any leaks. What I am parsing are multiple parameters. Each parameter has a name and a value. An example input would be:

parameterName = 500;

The name is parameterName and it is of type integer with a value of 500.

I am successfully able to parse one of these without memory leaks. However, doing a second parameter will cause leaks and I know why: It's the multiple uses of malloc on my parameter name.

Take a look at the parsing code:

int main()
{
    int x;
    char c;
    char *nameTemp;
    int hasName = 0;
    int hasEqual = 0;

    /* ParameterManager values */
    ParameterManager *pm;
    pm = PM_create(500);
    if((PM_manage(pm, "name", INT_TYPE, 1)));

    while((x = getchar()) != EOF)
    {
        /* Cast int to char */
        c = (char)x;

        /* Whitespace state */
        if((isspace(c)))
        {
            c = whitespace();
        }

        /* Comment state */
        if(c == '#')
        {
            c = comment();          
        }

        /* Name state */    
        if(((isalnum(c)) && hasEqual == 0 && hasName == 0))
        {
            nameTemp = name(c);
            printf("Name: %s\n", nameTemp);
            hasName = 1;
        }

        /* Equal state */
        if(c == '=' && hasName == 1 && hasEqual == 0)
        {
            hasEqual = 1;
        }

        /* Value state */
        if((isalnum(c)) && hasName == 1 && hasEqual == 1)
        {
            getValues(c, nameTemp, pm->t_List, pm->m_List);
            hasName = 0;
            hasEqual = 0;
        }
    }

    free(nameTemp);
    if((PM_destroy(pm)) && DEBUG) printf("Success destroying PM.\n");
    return 0;
}

The line nameTemp = name(c), under /* Name state */, returns an allocated string. This string is later passed to do other work. However, since this whole parsing idea is in a loop, multiple mallocs to the same string will be made. I can only free nameTemp once but there are multiple mallocs on that name. How can I reuse nameTemp over and over without causing any leaks?

Here is a piece of code (in function name()) where nameTemp is allocated:

 /* Make sure temp is not NULL before mallocing */
    if(temp[0] != '\0')
    {
        returnName = malloc(sizeof(char)*strlen(temp)+1);
        strncpy(returnName, temp, strlen(temp)+1);
        temp[0] = '\0';
        return returnName;
    }

I apologize if a few things are unclear. I'm trying to be as general as I can so if you need more clarification please let me know.

Plaidypus
  • 81
  • 9

2 Answers2

2

malloc() does not keep track of allocated blocks. You need to locate all the places where you're finished dealing with the memory you requested, and free() it there.

If I read your code correctly, that would be at the end of your while loop's body.

Edit : pulling up the comments.

It's undefined behaviour to try and use a block of memory that you already free()'d.

However, the pointer you use to keep a handle on the block is just a regular pointer, and won't go stale after you pass it to free(). In fact, it won't budge at all, since free() takes it by copy.

It is thus common to see said pointer set to NULL after it has been passed to free(), to ensure one does not accidentally reuse the now-unusable block.

You can then very well reuse it to be a handle on a brand new block returned by malloc(), as usual.

Quentin
  • 62,093
  • 7
  • 131
  • 191
  • Isn't it illegal to free a pointer if I will be using it again? If I put **free(tempName)** at the end of my body, the next time I collect a parameter name, it will give me an error. Or am I not on the same page as you? – Plaidypus Feb 07 '15 at 23:57
  • 1
    @Plaidypus: You're confusing the "pointer" (which is a variable that points to some memory somewhere) with the actual block of allocated memory. It's illegal to free *the same block of memory* twice. But you can reuse the variable for different things, and in a loop like this, it makes perfect sense to. – Ben Zotto Feb 08 '15 at 00:05
  • 1
    @Plaidypus No, that's perfectly fine. Using the same pointer **address** after you've freed it is undefined behavior (unless it is returned again by `malloc`, which is entirely possible), but the variable containing the pointer address has automatic storage duration and is usable until the function exits. – IllusiveBrian Feb 08 '15 at 00:05
  • @BenZotto That makes sense! I had forgotten about my `free(nameTemp)` outside of the loop, and that was causing me problems. Thanks everyone for the explanations. I should seriously spend an evening reading up on dynamic memory in C. – Plaidypus Feb 08 '15 at 00:14
  • 2
    @Plaidypus Once you've understood how dynamic memory and pointers work, you'll have seen a good chunk of C. It's a tiny language ! – Quentin Feb 08 '15 at 00:17
2

nameTemp = name(c); causes an inevitable leak, when nameTemp stores a pointer which is not saved somewhere else and is also not freed at this time.

There are quite a few options to avoid this (depends on what you are trying to achieve and how much you are willing to change your code structure).

Three possibilities (ordered from least amount of code change to most):

  1. Free the memory before its assigned again (and free it again at the end of the program)
free(nameTemp);
nameTemp = name(c);
  1. Free the memory when it becomes obsolete
/* Value state */
if((isalnum(c)) && hasName == 1 && hasEqual == 1)
{
    getValues(c, nameTemp, pm->t_List, pm->m_List);
    hasName = 0;
    hasEqual = 0;
    free(nameTemp);
    nameTemp=NULL;
}
  1. Use a general purpose buffer, allocate a big enough junk of memory at the beginning and free it at the end again.
char* nameTemp;
nameTemp = (char*)malloc(512); //Or any other size, just check that its actually big enough before writing to it, otherwise buffer overflow errors will occur.

// Somwhere in your program
write_name( nameTemp, 512 , c ); // Pass the buffer to be filled by the function, instead of returning a new pointer.

// At the end of your program
free(nameTemp);
nameTemp = NULL; //Prevent access of freed memory.
MarvinPohl
  • 156
  • 3