Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

    replace_str(str1, str3, str2,buff);

    puts(buff);

    return 0;
}
share|improve this question
add comment

1 Answer

  • 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.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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