3

This example works but I think that the memory leaks. Function used in the simple web server module and thus shared memory grows if you use this function.

    char *str_replace ( const char *string, const char *substr, const char *replacement ){
      char *tok = NULL;
      char *newstr = NULL;
      char *oldstr = NULL;
      if ( substr == NULL || replacement == NULL ) return strdup (string);
      newstr = strdup (string);
      while ( (tok = strstr ( newstr, substr ))){
        oldstr = newstr;
        newstr = malloc ( strlen ( oldstr ) - strlen ( substr ) + strlen ( replacement ) + 1 );
        memset(newstr,0,strlen ( oldstr ) - strlen ( substr ) + strlen ( replacement ) + 1);
        if ( newstr == NULL ){
          free (oldstr);
          return NULL;
        }
        memcpy ( newstr, oldstr, tok - oldstr );
        memcpy ( newstr + (tok - oldstr), replacement, strlen ( replacement ) );
        memcpy ( newstr + (tok - oldstr) + strlen( replacement ), tok + strlen ( substr ), strlen ( oldstr ) - strlen ( substr ) - ( tok - oldstr ) );
        memset ( newstr + strlen ( oldstr ) - strlen ( substr ) + strlen ( replacement ) , 0, 1 );
        free (oldstr);
      }
      return newstr;
    }

Bdfy
  • 23,141
  • 55
  • 131
  • 179

4 Answers4

11

One problem I can see is that if the replacement string contains the search string, you'll loop forever (until you run out of memory).

For example:

char *result = str_replace("abc", "a", "aa");

Also, doing another malloc/free every time you replace one instance is pretty expensive.

A better approach would be to do exactly 2 passes over the input string:

  • the first pass, count how many instances of the search string are present

  • now that you know how many matches, compute the length of your result & malloc once:

    strlen(string) + matches*(strlen(replacement)-strlen(substr)) + 1

  • make a second pass through the source string, copying/replacing

David Gelhar
  • 27,873
  • 3
  • 67
  • 84
1

This will replace all occurrence of "str" with "rep" in "src"...

void strreplace(char *src, char *str, char *rep)
{
    char *p = strstr(src, str);
    do  
    {   
        if(p)
        {
            char buf[1024];
            memset(buf,'\0',strlen(buf));

            if(src == p)
            {
                strcpy(buf,rep);
                strcat(buf,p+strlen(str));  
            }
            else
            {
                strncpy(buf,src,strlen(src) - strlen(p));
                strcat(buf,rep);
                strcat(buf,p+strlen(str));
            }

            memset(src,'\0',strlen(src));
            strcpy(src,buf);
        }   

    }while(p && (p = strstr(src, str)));
}
Sunny Shukla
  • 537
  • 1
  • 6
  • 17
1

Explain this part:

if ( substr == NULL || replacement == NULL ) return strdup (string);

Why do you return a copy of the existing string? This will leak memory, and it's unnecessary.

You also never free the duplicate if the while loop is skipped (i.e. the condition is never met).

EboMike
  • 76,846
  • 14
  • 164
  • 167
  • In fairness the existing string is always copied & a new string returned - either when there are replacements (`return newstr;`) or when there aren't (`return strdup(string);`). In both cases freeing the original input `string` (not a great parameter name) cuts down on memory usage, assuming the original script no longer needs it (or alternatively replace the original `string` in the function and return void). – Rudu Sep 07 '10 at 14:56
  • 1
    Quite honestly, as a programmer I'd feel kind of duped if I passed a const char * to a function and it frees it up for me :) – EboMike Sep 07 '10 at 15:18
0
  • strdup is not C89/C99, therefore your code => no ANSI C
  • better make the NULL-test direct after malloc

here an example, only with one new memoryblock:

/* precondition: s!=0, old!=0, new!=0 */
char *str_replace(const char *s, const char *old, const char *new)
{
  size_t slen = strlen(s)+1;
  char *cout = malloc(slen), *p=cout;
  if( !p )
    return 0;
  while( *s )
    if( !strncmp(s, old, strlen(old)) )
    {
      p  -= cout;
      cout= realloc(cout, slen += strlen(new)-strlen(old) );
      p  += strlen( strcpy(p=cout+(int)p, new) );
      s  += strlen(old);
    }
    else
     *p++=*s++;

  *p=0;
  return cout;
}
user411313
  • 3,930
  • 19
  • 16
  • You might want to cache the value of `strlen(old)`, it might be faster and computing it on each iteration. – jbernadas Sep 07 '10 at 17:57
  • 1
    The line `p += strlen( strcpy(p=cout+(int)p, new) );` causes undefined behavior - assigning twice to "p" without an intervening sequence point. Nicely obfuscated, though. – David Gelhar Sep 08 '10 at 03:44