Having issue with realloc in custom str_replace function. Invalid next size error

After looking online for a string replace function in C and finding so many examples that go through the entire string twice. First round to find how number of occurances of substitute in string, using that to malloc for a new string to include additional space for replace, then going through the search string again to get all but what's to be substituted out. I feel it's kind of silly to go through the string twice. Therefore, I'm trying to implement my own string replace, that only searches through the string for what's to be substituted, once.

Here is my code:

Code:

---

#include <stdio.h>
#include <string.h>
#include <stdlib.h>


char *strreplace(char * string, char * sub, char * replace);

int main(void){
        char * mystring = "Here is my string.";
        char * sub = " ";
        char * replace = "__++";

        char * newString = strreplace(mystring, sub, replace);
        printf("new string: %s\n", newString);
        return 0;
}

//my replace
char *strreplace(char *string, char *sub, char *replace)
{
        if(!string || !sub || !replace) return NULL; // we need all inputs

        char *result = (char*)malloc(1); // malloc memory for new string, will be realloc later
        if(result == NULL){
                printf("malloc returned null\n");
                return NULL;
        }
       
        char *pos = string; // set pointer of pos to start of string
        char *pos1; // pointer to search for sub
        while((pos1 = strstr(pos, sub))) // search for sub and point to start of memory location
        {
                int len = (pos1 - pos); // length = found memory locatino - beginning of string
                result = realloc(result, len + strlen(replace) + 1); // realloc result for string + replacemenet string + \0

                strncat(result, pos, len); // pull up to this location
                strcat(result, replace); //replace the found (strcat is fine because we already allocated the memory)
                pos = (pos1 + strlen(sub)); // update pos to find next occurance of sub in string
        }

        // if we aren't at the end of the original string, we need to get what's left of original string
        if(pos != (string + strlen(string)))
        {
                result = realloc(result, strlen(result) + strlen(pos) + 1); // realloc for what is left of original string
                strcat(result, pos); // get the rest of the original string
        }
       

        return result; // return new string
}

---

Here is the same code, but with execution + some syntax highlighting: Ideone.com | Online C Compiler & Debugging Tool

It works great, until it gets to grabbing whatever remains in the search string after the last found sub. The realloc throws a runtime error:

Code:

---

*** glibc detected *** ./a.out: realloc(): invalid next size: 0x00011008 ***
Aborted

---

From my understanding, this is from me going outside of the bounds of the heap and using memory I haven't allocated, or using a free'd pointer. I'm not seeing where either of these are happening in my code, and was wondering what the best way to go about figuring out where the error exactly occurs.

Thank you for any input.

Okay, so immediately after posting this, I think I may have found the source of the issue. strcat() in the while loop is causing the memory corruption. Commenting that out, and the realloc() error doesn't appear. Going to try and figure out why there isn't enough memory for strcat(), and am assuming it doesn't have space for the null terminator in the string.

Edit: I believe it's how much memory I'm realloc() in the while() is not enough. Still debugging.

Edit2: Found the error. It was in the realloc() in the while(). Originally, as seen from my original post, I'm only reallocating for the increase in memory size, not including the length of what is already currently in the new string. Here is the updated code:

Code:

---

#include <stdio.h>
#include <string.h>
#include <stdlib.h>


char *strreplace(char * string, char * sub, char * replace);

int main(void){
        char * mystring = "Here is my string.";
        char * sub = " ";
        char * replace = "__++";

        char * newString = strreplace(mystring, sub, replace);
        printf("new string: %s\n", newString);
        return 0;
}

//my replace
char *strreplace(char *string, char *sub, char *replace)
{
        if(!string || !sub || !replace) return NULL; // we need all inputs

        char *result = (char*)malloc(1); // malloc memory for new string, will be realloc later
        if(result == NULL){
                printf("malloc returned null\n");
                return NULL;
        }
       
        char *pos = string; // set pointer of pos to start of string
        char *pos1; // pointer to search for sub
        while((pos1 = strstr(pos, sub))) // search for sub and point to start of memory location
        {
                int len = (pos1 - pos); // length = found memory locatino - beginning of string
                result = realloc(result, strlen(result) + len + strlen(replace) + 1); // realloc result for string + replacemenet string + \0

                strncat(result, pos, len); // pull up to this location
                strncat(result, replace, strlen(replace)); //replace the found (strcat is fine because we already allocated the memory)
                pos = (pos1 + strlen(sub)); // update pos to find next occurance of sub in string
        }

        // if we aren't at the end of the original string, we need to get what's left of original string
        if(pos != (string + strlen(string)))
        {
                result = realloc(result, strlen(result) + strlen(pos) + 1); // realloc for what is left of original string
                strcat(result, pos); // get the rest of the original string
        }
       
        return result; // return new string
}

---

And here it is executed: http://ideone.com/UeTzqw

I actually made two changes. I changed strcat to strncat with n being strlen of replace string.

I also added strlen(result) in the realloc of the while.

So for now, at least with the example case in main(), it works and doesn't throw any errors. Haven't tested it thoroughly, but this answers my original question.

Two comments:

1. You are checking the return value of malloc, but you aren't checking realloc. Each time you call realloc, there is some chance that it won't work.
2. Imagine that your string is long and that you need to make many replacements. For example, replace instances of "<%name>" with your full company name in the string

"<html> <head> ... <%name> ... <%name> ... </html>"

In this general case, you ultimately need to count up how many times "<%name>" appears in the string before you know how long the result will be. This is why scanning through the string one time beforehand is not really a bad option. With your realloc idea, you ultimately have to call realloc each time <%name> appears in the string. In other words, if your idea is to save time or improve efficiency by doing it that way, I don't think it will pan out.

Code:

---

result = realloc(result, strlen(result) + len + strlen(replace) + 1); // realloc result for string + replacemenet string + \0

---

The first time you get to this line, result isn't initialised and strlen(result) could return anything.

Code:

---

if(pos != (string + strlen(string)))

---

Even if you would have managed to go through the string only one time during all the replacements (which you haven't because of all the strlen() and strncat() calls), strlen(string) will go through the whole string again in order to find the \0 and you are back at running through the string twice.

Bye, Andreas