Inserting strings into another string in C

Question

I'm implementing a function which, given a string, a character and another string (since now we can call it the "substring"); puts the substring everywhere the character is in the string. To explain me better, given these parameters this is what the function should return (pseudocode):

func ("aeiou", 'i', "hello")  ->  aehelloou

I'm using some functions from string.h lib. I have tested it with pretty good result:

char *somestring= "this$ is a tes$t wawawa$wa";
printf("%s", strcinsert(somestring, '$', "WHAT?!") );

Outputs:    thisWHAT?! is a tesWHAT?!t wawawaWHAT?!wa

so for now everything is allright. The problem is when I try to do the same with, for example this string:

char *somestring= "this \"is a test\" wawawawa";
printf("%s", strcinsert(somestring, '"', "\\\"") );

since I want to change every " for a \" . When I do this, the PC collapses. I don't know why but it stops working and then shutdown. I've head some about the bad behavior of some functions of the string.h lib but I couldn't find any information about this, I really thank any help.

My code:

#define salloc(size) (str)malloc(size+1) //i'm lazy
typedef char* str;

str strcinsert (str string, char flag, str substring)
{
    int nflag= 0; //this is the number of times the character appears
    for (int i= 0; i<strlen(string); i++)
        if (string[i]==flag)
            nflag++;
    str new=string;
    int pos;
    while (strchr(string, flag)) //since when its not found returns NULL
    {
        new= salloc(strlen(string)+nflag*strlen(substring)-nflag);
        pos= strlen(string)-strlen(strchr(string, flag));
        strncpy(new, string, pos);
        strcat(new, substring);
        strcat(new, string+pos+1);
        string= new;      
    }
    return new;
}

Thanks for any help!

Answer 1

Some advices:

• refrain from typedef char* str;. The char * type is common in C and masking it will just make your code harder to be reviewed
• refrain from #define salloc(size) (str)malloc(size+1) for the exact same reason. In addition don't cast malloc in C
• each time you write a malloc (or calloc or realloc) there should be a corresponding free: C has no garbage collection
• dynamic allocation is expensive, use it only when needed. Said differently a malloc inside a loop should be looked at twice (especially if there is no corresponding free)
• always test allocation function (unrelated: and io) a malloc will simply return NULL when you exhaust memory. A nice error message is then easier to understand than a crash
• learn to use a debugger: if you had executed your code under a debugger the error would have been evident

Next the cause: if the replacement string contains the original one, you fall again on it and run in an endless loop

A possible workaround: allocate the result string before the loop and advance both in the original one and the result. It will save you from unnecessary allocations and de-allocations, and will be immune to the original char being present in the replacement string.

Possible code:

// the result is an allocated string that must be freed by caller
str strcinsert(str string, char flag, str substring)
{
    int nflag = 0; //this is the number of times the character appears
    for (int i = 0; i<strlen(string); i++)
        if (string[i] == flag)
            nflag++;
    str new_ = string;
    int pos;
    new_ = salloc(strlen(string) + nflag*strlen(substring) - nflag);
    // should test new_ != NULL
    char * cur = new_;
    char *old = string;
    while (NULL != (string = strchr(string, flag))) //since when its not found returns NULL
    {
        pos = string - old;
        strncpy(cur, old, pos);
        cur[pos] = '\0';             // strncpy does not null terminate the dest. string
        strcat(cur, substring);
        strcat(cur, string + 1);
        cur += strlen(substring) + pos; // advance the result
        old = ++string;                 // and the input string
    }
    return new_;
}

Note: I have not reverted the str and salloc but you really should do.