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 <stdio.h>
#include <string.h>

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

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

    int ret = 0;

    TempStr = strstr(SrcStr, SubStr);

    if ( TempStr )

        strncpy(buffer, SrcStr, TempStr-SrcStr);

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

        ret = replace_str(buffer, SubStr, RepStr, FinalStr);

        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] = "&apos;"; 
    char str3[3] = "\'" ;
    char buff[256] ={0};

    replace_str(str1, str3, str2,buff);


    return 0;

2 Answers 2

  • 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.

In addition to the many issues pointed out by @Fabian, you are overflowing your Tmp2 buffer as soon as you create it:

char *Tmp2 = malloc(250);

You're creating a buffer of 250 bytes and then initialising 256 bytes which will overflow into whatever is next in memory.

You also seem to be making things unnecessarily complicated.

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

Could simply be:

char str1[256] = "pra\'dipta\'kumar\'rout";

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.