C buffer overflow

Question

I tried to make a function that replaces all occurrences of str1 in a text t with str2 but I keep getting a "buffer overflow" error message. Can you please 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,r-p); //copy r-p 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 used 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 the 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;
}

printed string:

I expected cxyzwnxyz4jkxyz

Stepping through this code in a debugger or printing out the value of variables each time through the loop should make it fairly clear where the problem lies. — bta
– bta, Commented Aug 20, 2012 at 22:48
sizeof(char) is by definition 1 in C. While it's a minor style nit, it suggests deeper problems in your understanding of the language. — mlp
– mlp, Commented Aug 20, 2012 at 23:29

pb2q · Accepted Answer · 2012-08-20 22:58:39Z

7

It looks like you've got your strncpy arguments mixed up: the second argument is the source string, not the limit on the number of chars to copy, which should be the third argument:

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

Furthermore, you want to use strcat instead of strcpy. Using strcpy, you'll just overwrite the contents of the result with the replacement string, every time. Using strcat, be sure to initialize the result with \0 before starting.

Finally, you're returning a reference to a local variable x from your function: you can't do this as the memory isn't usable after the function returns.

edited Aug 20, 2012 at 22:58

answered Aug 20, 2012 at 22:28

pb2q

59.9k19 gold badges150 silver badges152 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Cristi Over a year ago

Thank you! I will try with dynamic allocation

AnT stands with Russia · Accepted Answer · 2012-08-20 23:01:40Z

Your code contains quite a few weird bugs.

Firstly, x is a pointer to your destination buffer. For come reason you are doing all your copyings directly to x, i.e. everything is copied to the very beginning of the buffer, overwriting previously copied data. This doesn't make any sense at all. Whay are you doing this? You need to create a dedicated pointer to would keep the current destination position in x and write data to that position (instead of writing it to x).

I see that you edited your code and replaced copying with concatenation. Well... Even though it might fix the problem, this is still 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 on uninitialized buffer x. You need to initialize x as an empty string first.

Secondly, there's a more subtle problem with your search for replacement string. At the end of the cycle you continue the search from the next symbol r=strstr(r+1,str1), i.e. you increment the search position by only 1. I'm not sure this is what you want.

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

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

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

and see what happens.

Collectives™ on Stack Overflow

C buffer overflow

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related