3

I am learning C and I came across a problem while manipulating strings. In a problem I was solving I was supposed to write a function to take a string and a character and delete all occurrences of the given character, and then I had to return the modified string. The function I wrote is this:

char *strdelc3(char *s, char ch){
for(int i=0,j=0; i!=strlen(s)+1; ++i)
 if(s[i]!=ch){
  s[j]=s[i];
  ++j;
 }
return s;
}

And when I pass a string and a character as arguments:

main(){
char s[20]="mary";
puts(strdelc3(s,'r'));
}
The output is: Segmentation fault(core dumped),

which by my research means I am accessing memory that does not belong to me. The solutions had this code:

char *strdelc4(char *s, char ch){ /*Correct*/
int i,j;
for(i=0, j=0; s[i]!='\0'; ++i)
 if(s[i]!=ch){
  s[j]=s[i];
  ++j;
 }
s[j]='\0';
return s;
}

Which is basically equal to mine, however this piece works fine! Since the two codes are so similar I don't see anything wrong with mine... I have already studied both but I don't see what is the problem with mine... Could someone help?

Jason Aller
  • 3,541
  • 28
  • 38
  • 38
Noslen
  • 35
  • 5
  • 3
    Why `i!=strlen(s)+1` in the first example? I think that's where the problem is. Why add one? – Fred Larson Sep 16 '20 at 21:49
  • There are two differences between the functions. Try them one by one to see why each one introduces a different problem. – dxiv Sep 16 '20 at 21:51
  • 3
    The problem is that when `i` is `strlen(s)`, you'll move the zero byte, and the string length will decrease, so `i` will never be equal to `strlen(s)+1`. It's always a bad idea to put `strlen` in a loop conditional (it's a huge waste of time). But in this case, it's especially bad. – user3386109 Sep 16 '20 at 21:52
  • 1
    Anyway, take a simple string few characters long and run your program on it with a debugger. This is what we usually do if not able to spot the problem by looking at the code. – Eugene Sh. Sep 16 '20 at 21:57
  • Or forego the indexing and [just use pointers](https://pastebin.com/Lec9GexD). – WhozCraig Sep 16 '20 at 21:59
  • 2
    Ahhhh, wow thank you user3386109, now I understand! when i=5, strlen(s)+1=4, and i will keep on increasing without being equal to my condition. – Noslen Sep 16 '20 at 22:00
  • Thank you all who responded in such a short time. The other suggestions are great. I can't still use a debugger but someday I will try. – Noslen Sep 16 '20 at 22:02
  • Related Q&A that has a similar correct solution to properly process a string: https://stackoverflow.com/questions/68422565/write-a-c-program-which-capitalizes-the-first-letter-of-each-word-in-a-sentenc – Gabriel Staples Jul 18 '21 at 17:30

2 Answers2

3

The problem is in your loop conditional:

i!=strlen(s)+1

You're attempting to use strlen(s)+1 here to avoid having to add the null byte. But in doing so, strlen(s) changes once you move the terminating null byte.

On the first 4 iterations through the loop, strlen(s) is 4. On the next iteration, i is 4 and strlen(s)+1 is 5 so you enter the loop again. You then move the null byte. Now on the following iteration, strlen(s) is 3 and i is 5. The conditional is still true so you keep going, walking off the end of the string. This invokes undefined behavior which in this case causes a crash.

The second piece of code addresses this issue by explicitly looking for the null byte based on the index of i and appending a null byte to the resulting string after the loop.

dbush
  • 205,898
  • 23
  • 218
  • 273
0

An even simpler version of the code would use the do - while loop instead of for():

char *strdelc5idx(char *s, char ch){
    int i=0, j=0;

    do {
        if (s[i] != ch)
            s[j++] = s[i];
    } while (s[i++] != 0);

    return s;
}

This will copy the string-terminating NUL character before testing it, so you needn't have a separate instruction for it. However, that requires deferring the i++ incrementation so that the loop condition at the end of an iteration tests the same character which was copied in the iteration. As a result the i++ and j++ do no longer appear together, which may make this code less legible at a first glance.

An equivalent pointer version:

char *strdelc5ptr(char *s, char ch){
    char *d = s, *f = s;

    do {
        if (*f != ch)
            *d++ = *f;
    } while (*f++);

    return s;
}
CiaPan
  • 9,381
  • 2
  • 21
  • 35