Why does freeing the dynamically allocated memory create issue here?

Question

I have this code:

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

char* creatString();
void printWordsThatStartWithLETTER(char letter, char *str);

void main()
{
    char *strEx1;
    char letter;
    strEx1=creatString();
    printf("Enter a letter:\n");
    scanf("%c",&letter);
    printWordsThatStartWithLETTER(letter, strEx1);
    free(strEx1);
}

char* creatString()
{
    char *str, *strTemp;
    int size;
    strTemp=(char*)malloc(256);
    printf("enter your string:\n");
    flushall();
    gets(strTemp);
    size = strlen(strTemp);
    str=(char*)malloc(size);
    strcpy(str,strTemp);
    //puts(str);
    free(strTemp);
    return str;
}
void printWordsThatStartWithLETTER(char letter, char *str)
{
    int sizeOfStrinf, i;
    sizeOfStrinf = strlen(str);
    for(i=0;i<sizeOfStrinf;i++)
    {
        if((str[i]==letter)||(str[i]==letter-32))
        {
            if(i==0)
            {
                while(str[i]!=32)
                {
                    printf("%c",str[i]);
                    i++;
                }
                printf("\n");
            }
            else
                if(str[i-1]==32)
                {
                    while(str[i]!=32)
                    {
                        printf("%c",str[i]);
                        i++;
                    }
                    printf("\n");
                }
        }
    }
}

it wont free strEx1, I have overflow. How can I free strEx1 properly?

`warning: ‘gets’ is deprecated (declared at /usr/include/stdio.h:638) [-Wdeprecated-declarations]` — LPs, May 20 '16 at 07:27
Welcome to Stack Overflow! [Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, May 20 '16 at 07:30
It's non-standard, but `strdup()` is widely available: `str = strdup(strTemp)` — Andrew Henle, May 20 '16 at 07:31

score 4 · Answer 1 · edited May 23 '17 at 12:31

You have the actual issue in your code where you do

size = strlen(strTemp);
str=(char*)malloc(size);

basically, you're one element short, no space for null-terminator. So later using strcpy()

 strcpy(str,strTemp);

causes out of bound access which invokes undefined behavior.

You should do something like

 str = malloc(size + 1);

That said,

Please see this discussion on why not to cast the return value of malloc() and family in C..
Always check the return value of malloc() for success before using the return value to avoid null-pointer dereference.
You must not be using gets().

score 0 · Accepted Answer · answered May 20 '16 at 10:56

You are allocating memory that is one byte too short. Add one

size = strlen(strTemp);    
str=(char*)malloc(size+1);

or instead of strcpy use strncpy

size = strlen(strTemp);
str=(char*)malloc(size);
strncpy(str,strTemp,size);

The strncpy function copies the initial count characters of strSource to strDest and returns strDest. If count is less than or equal to the length of strSource, a null character is not appended automatically to the copied string. If count is greater than the length of strSource, the destination string is padded with null characters up to length count. The behavior of strncpy is undefined if the source and destination strings overlap

Why does freeing the dynamically allocated memory create issue here?

2 Answers2