C buffer overflow

Question

I tried to make a function that replaces all occurrences of str1 in text t with str2 , but I continue to receive a buffer overflow error message. Could you tell me what is wrong with my function?

 #include <stdio.h> #include <string.h> #include <assert.h> //replace all *str1 in *t with *str2, put the result in *x, return *x char * result(char *str1,char *str2,char *t) { char *x=NULL,*p=t,*r=t; x=malloc(400*sizeof(char)); assert(x!=NULL); x[0]='\0'; r=strstr(t,str1); //r is at the first occurrence of str1 in t, p is at the beginning of t while(r!=NULL) { strncat(x,p,rp); //copy rp chars from p to x strcat(x,str2); //copy str2 to x p=r+strlen(str1); //p will be at the first char after the last occurrence of str1 in t r=strstr(r+strlen(str1),str1); //r goes to the next occurrence of str1 in t } strcat(x,p); return x; } 

I did not use the gets() function to read any char array.

My compiler is gcc version 4.6.3

---

I updated the code, it works, but the result is not as expected.

main() Function:

 int main(void) { char *sir="ab",*sir2="xyz",*text="cabwnab4jkab",*final; final=result(sir,sir2,text); puts(final); free(final); return 0; } 

print line:

 b 

I was expecting cxyzwnxyz4jkxyz

Answer 1

It looks like you have strncpy arguments: the second argument is the source string, and not the number of characters to copy, which should be the third argument:

  strncpy(x, p, r - p); // copy r - p chars from p to x 

Also, you want to use strcat instead of strcpy . Using strcpy , you will simply overwrite the contents of the result with the replacement string each time. Using strcat , be sure to initialize the result with \0 before starting it.

Finally, you return a reference to the local variable x from your function: you cannot do this, since memory will not be used after the function returns.

Answer 2

Your code contains quite a few strange errors.

First, x is a pointer to your target buffer. For the reason you make all your copies directly on x , i.e. Everything is copied at the very beginning of the buffer, overwriting previously copied data. It makes no sense. Do you see this? You need to create a highlighted pointer to save the current destination to x and write data to that position (instead of writing to x ).

I see that you edited your code and replaced copying with concatenation. Well ... Although this may solve the problem, it is still a bad design. strcat / strncat functions have no place in good C code. Anyway, your code is still broken, since you are trying to use strcat functions in an uninitialized x buffer. First you need to initialize x as an empty string.

Secondly, there is a more subtle problem with finding a replacement string. At the end of the loop, you continue the search from the next character r=strstr(r+1,str1) , that is, you increase the search position only by 1. I'm not sure if this is what you want.

Consider aaaa as input and the request to replace aa with bc . How many replacements do you want to make in this case? How many occurrences of aa are in aaaa ? 2 or 3? If you want to get bcbc as the result (2 replacements), you should increase r by strlen(str1) , and not by 1.

In fact, in the current implementation, you set p=r+strlen(str1) , but continue to search at position r+1 . This will lead to completely meaningless results with overlapping occurrences of the search string, as in my example. try it

 char *str1="aa",*str2="xyz",*text="aaaa",*final; final=result(str1,str2,text); 

and see what happens.