strlen and free memory

Question

I allocated memory to a pointer to the maximum size of characters it could have. Then I had to write code that will change its values depending on the value that was read from the file and I needed to know what is the length of the value in the pointer, so I used strlen() function.

I got what I needed. The problem occured when I tried to free the memory of that pointer. The program crashed, I'm assuming im doing something "ilegal" and would like to know why and how to fix it.

here is part of the code:

char *StudID = (char*)malloc(sizeof(char)*15);
char *StudIDcpy = (char*)malloc(sizeof(char) * 15);
fread(stud[i].ID, sizeof(char), 4, in);
stud[i].ID[4] = '\0';
IDtemp = atoi(stud[i].ID);//convert ID string to integer and store value in IDtemp
StudIDcpy = itoba(IDtemp);//convert integer to binary number as a string
strcpy(StudID, StudIDcpy);
IDtemp = strlen(StudIDcpy);
free(StudIDcpy); // <---- I BELIEVE THIS IS WHERE IT CRASHES

Here is my itoba() function:

char *itoba(int a){
    int i = 0, j;
    char temp[15];
    while(a){
        if (a % 2)temp[i] = '1';
        else temp[i] = '0';
        i++;
        a = a / 2;
    }
    temp[i] = '\0';
    for (j = 0; j < i / 2; j++)swapc(&temp[j], &temp[i - j-1]);
    return temp;
}

By the way I know I don't have to write sizeof(char) because it is equal to 1, but I write it anyways so I remember what value should be put there.

`return temp;` returns a pointer to a local variable, which disappears when the function exits. — Thomas Padron-McCarthy, Mar 30 '16 at 20:54
You leak the memory allocated on your first 2 lines, you call `free` on local buffer `temp` which doesn't even exist at that point... — M.M, Mar 30 '16 at 20:56
so because temp is not a pointer and StudIDcpy is getting its address so i dont need to allocate memory for it? and also dont need to free it? — Dani, Mar 30 '16 at 20:57
`temp` is on the stack. That disappears at the end of the function. So the pointer is no longer valid — Ed Heal, Mar 30 '16 at 21:00
See this famous Stackoverflow question/answer: http://stackoverflow.com/q/6441218/12711 — Michael Burr, Mar 30 '16 at 21:03
@tikanti-man `char temp[15];` allocates memory, and the memory is automatically freed when the function returns. You cannot write `strcpy(StudID, StudIDcpy);` because `StudIDcpy` is pointing to memory that has already been freed. — M.M, Mar 30 '16 at 21:03
COOL!!! i understand... now i need to think of a different way to make my code work... thanks for the help! — Dani, Mar 30 '16 at 21:10
I changed the 'char *itoba(int a)' to 'void itoba(int a,char *temp)' I think this is the best way to go... — Dani, Mar 30 '16 at 21:29
OK but you should also supply length as argument, otherwise the function has no way of avoiding a buffer overflow — M.M, Mar 30 '16 at 21:58
length of what? what would i do with hit? (by the way thank you very much for your help :) ) — Dani, Mar 30 '16 at 22:02
I made sure the integer send to the function is not longer then a 4 digit number, so maximum string value could be 14 zeros or ones... so there could not be a buffer overflow. but thanks for your help. — Dani, Mar 31 '16 at 08:07

nalzok · Accepted Answer · 2016-03-31T01:14:44.517

In your itoba() function, temp, a local array, which decays to a pointer to local variables, is returned.

After a function returns, its local variables are "free"ed immediately, allowing these memory space to be reused by someone else. Consequently, values held by them will soon be overridden by other values on the stack.

You can rewrite itoba() like this:

char *itoba(int a)
{
    int i = 0, j;
    char *temp = malloc(15); // <--- This line is different
    while(a){
        if (a % 2)
            temp[i] = '1';
        else
            temp[i] = '0';
        i++;
        a = a / 2;
    }
    temp[i] = '\0';
    for (j = 0; j < i / 2; j++)
        swapc(&temp[j], &temp[i - j -1]);
    return temp;
}

BTW: You should remove char *StudIDcpy = (char*)malloc(sizeof(char) * 15);, because the pointer value returned by malloc() is later discarded by itoba(IDtemp);. As a result, the memory allocated to StudIDcpy by this malloc() will never be freed, causing memory leak.

strlen and free memory

1 Answers1