freeing substring without doing double free in c

Question

Yesterday I asked a similar question regarding how to free allocated memory for a sub-string. Now I have one more question regarding the same problem (involving a set of conditions), how could I free the following sub-string without doing double free?

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

struct st_ex {
    char product[16];
    float price;
};
struct st_temp {
    char *prod;
};

char *temp = NULL;

// from stackoverflow
char* substr( const char* source, size_t start, size_t end )
{
    char* dest = malloc( end - start + 1) ;
    memcpy( dest, &source[start], end - start ) ;
    dest[end - start] = 0 ;
    return dest ;
}

int main()
{
    struct st_ex structs[] = {{"mp3 player", 2.0f}, {"plasma tv", 20.0f},
                              {"notebook", 10.0f},  {"smartphone", 49.9f},
                              {"dvd player", 10.0f}, {"matches", 0.2f }};
    struct st_temp **temp_struct;

    size_t j, i;
    temp_struct = malloc(sizeof *temp_struct * 6);
    for (j = 0; j < 6; j++)
        temp_struct[j] = malloc(sizeof *temp_struct[j]);

    size_t structs_len = sizeof(structs) / sizeof(struct st_ex);

    // NOTE: that structs_len may vary in size - not just 6 
    for(i=0; i<structs_len; i++){
        if (i == 0)
            temp_struct[i]->prod = "+";
        else if(i == 1)
            temp_struct[i]->prod = "Bar";
        else if(i == 5)
            temp_struct[i]->prod = "Foo";
        else {
            temp = substr(structs[i].product, 0, 4);
            temp_struct[i]->prod = temp;
        }
    }
    for(i=0; i<6; i++ )
        printf("%s\n",temp_struct[i]->prod);

    for(i = 0; i < 6; i++ ){
        /* can I do something like this? */
        /*if (i != 0 || i != 1 || i != 5)*/
        free(temp_struct[i]->prod);
        free(temp_struct[i]);
    }
    free(temp_struct);
    return 0;
}

Answer 1

Yes, although you will want to uncomment that if, and change the conditions in the if to be joined with && rather than || (otherwise it will always be true -- every number is either not equal to zero or not equal to one!)

The substrings stored in temp_struct[i]->prod for i other than 0, 1, and 5, were allocated inside the substr function with malloc, so you can and should deallocate them with free.

Similarly, each temp_struct element was allocated with malloc, and so can and should be deallocated with free.

I'm not sure where you think the double free would be coming from. Are you thinking that when you call free(tmp_struct[i]) the memory pointed to by tmp_struct[i]->prod will also be freed? That isn't the case. When you free a pointer to a structure that contains pointers, the memory for the structure's pointers themselves is deallocated (being that it is part of the structure), but the memory being pointed to by those pointers is not, and must be deallocated seperately (since it is external to the structure). Aside from the mistake in the if condition, the way you have it written is the correct way to do this.

Answer 2

Substrings do not take up extra memory. They are pointers to parts of already existing strings.

Answer 3

Yes, given that your substr is allocating the memory for the substring with malloc, it's reasonable (necessary, really) to free that memory when you're done with it. That said, I think the way you're doing things right now is extremely fragile and error-prone (to put it mildly). If you have any choice at all, I'd allocate the strings for all the prod members the same way -- if you can't allocate them all statically, then allocate them all dynamically, so when you're freeing structures, you can do so uniformly. Trying to assure that you keep the subscripts matched up to free prod if and only if it was allocated dynamically is practically begging for trouble.

Answer 4

The problem is that sometimes you set temp_struct[i]->prod to a quoted string ("Bar") which you cannot free and sometimes to the result of a substr call, which you must free.

The easiest solution is to always set it to a string that you must free.

  temp_struct[i]->prod = new_string("Bar");

where

char* new_string( const char* source )
{
    char* dest = malloc( strlen(source) + 1 ) ;
    strcpy(dest, source);        
    return dest ;
}

or, you have to keep track if you need to free or not

 struct st_temp {
     char *prod;
     int prod_must_be_freed;
 };

set prod_must_be_freed to 0 or 1 and check that before you free.

And, finally, the whole thing would be improved by using functions to manipulate these structs rather than just fiddling with them directly. Then you could make a free_st_temp(st_temp*) that checked if prod should be freed, and then freed the struct. Your loop would be

for(i = 0; i < 6; i++ ){    
    free_st_temp(temp_struct[i]);
}

Answer 5

You have an additional problem. When you do temp_struct[i]->prod = "Bar"; You are assigning a const char* to prod. That pointer cannot be freed (the most likely outcome is a crash). So if you want to have your code set up this way such that prod can point to either dynamic memory that you got from malloc or to a constant string literal, you need to also keep track of which one it is and only free the dynamic memory.

The condition in your comment would technically work, but would be very poor form. The best idea is to not mix and match string types in the same pointer. But if you insist on doing it that way, then an improvement would be to add another variable to your struct that is set true when prod needs to be freed and false when it does not.