Optimizing string replacement program that uses recursion

I have the following string replacement program for a string of size 256:

I am replacing the string


"\'"

with the string


'

Please suggest any modifications if needed or any mistake I have done.

#include 
#include 

int replace_str(char *SrcStr, char *SubStr, char *RepStr, char *FinalStr )
{

    static char buffer[256]="";
    char *TempStr= NULL;
    char *Tmp2 = malloc(250);
    memset(Tmp2,0,256);

    int ret = 0;

    TempStr = strstr(SrcStr, SubStr);

    if ( TempStr )
    {
        memcpy(Tmp2,TempStr,strlen(TempStr));

        strncpy(buffer, SrcStr, TempStr-SrcStr);

        buffer[TempStr-SrcStr] = '\0';
        sprintf(buffer+(TempStr-SrcStr), "%s%s", RepStr, Tmp2+strlen(SubStr));

        puts(buffer);
        free(Tmp2);
        ret = replace_str(buffer, SubStr, RepStr, FinalStr);
    }
    else
    {

        memcpy(FinalStr, buffer , strlen(buffer));

    }
    return ret;
}

int main(void)
{
    char *str1 = malloc(256);
    memset(str1, 0 ,256);
    memcpy(str1, "pra\'dipta\'kumar\'rout",strlen("pra\'dipta\'kumar\'rout"));

    char str2[10] = "'"; 
    char str3[3] = "\'" ;
    char buff[256] ={0};

    replace_str(str1, str3, str2,buff);

    puts(buff);

    return 0;
}

• Many strange numbers. Why is Tmp2 only 250 long?

• Why is buffer static?

• Why is Tmp2 not freed in the else branch? Most likely the malloc() should also only happen in the then branch.

• The replace_str function should take a length argument for FinalStr, now this might cause a buffer overflow.

• Your naming is very strange: Both TmpStr and Tmp2 are temporary strings, but they are named quite different. Better give them proper names.

• if (TmpStr) uses implicit boolean conversion of pointers. That's bad practice.

• Your variable naming convention is inconsequent, some are UpperCamelCase, some are lower case.

• Why is there a call to puts? The function should work on a string, not print on stdout.

• Now that I think about it: Using fixed size malloc is generally stupid.

Either make that dependent on the actual needs or just use char[256].

• Also you might want to use calloc instead of malloc.

• Potential buffer overflow in memcpy(Tmp2,TempStr,strlen(TempStr));

• Potential buffer overflow in sprintf(buffer+(TempStr-SrcStr), "%s%s", RepStr, Tmp2+strlen(SubStr));

• Also why are you using strncpy and sprintf for the same purpose (string concatenation)? You might want to look at strncat.

• Unnecessary memory eating: The temporary strings are not needed for the recursion, better free before.

• Similar for buffer.

StackExchange Code Review Q#54052, answer score: 4