C code removing wrong character from string

Question

This code is supposed to remove any leading spaces from the given string, and it was working correctly. Then, for seemingly no reason at all, it started removing characters in the middle of the word. In this example the word "CHEDDAR" is given, which has no leading spaces so it should be passed back the same as it was input, however it's returning "CHEDDR" and I have no idea why. Does anyone know how this is even possible? I assume it has to do with pointers and memory, but I am not fluent in C and I need some help. Runnning on RHEL. Thanks.

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

#define REMOVE_LEADING_SPACES(input)                       \
           {                                            \
           stripFrontChar( input, ' ' );                         \
           }


char *stripFrontChar(char *startingString, char removeChar) {
    while (*startingString == removeChar)
        strcpy(startingString, startingString + 1);
    return (startingString);
}

void main(argc, argv)
    char **argv;int argc; {
    char *result = "CHEDDAR";
    REMOVE_LEADING_SPACES(result);
    printf("%s\n", result);
}

EDIT: It's a little late now but based on the comments I should have shown that the word (CHEDDAR I used as an example) is read from a file, not a literal as shown in my code. I was trying to simplify it for the question and I realize now it's a completely different scenario, so I shouldn't have. Thanks, looks like I need to use memmov.

EDIT2: There actually is a space like " CHEDDAR", so I really just need to change it to memmov, thanks again everyone.

I'm not sure how that's happening here, but [the strcpy docs say](http://www.cplusplus.com/reference/cstring/strcpy/) "destination ... should not overlap in memory with source" — Rup, Jan 07 '20 at 13:53
You should be able to remove all leading spaces in a single copy rather than repeatedly copying anyway: work out what offset you need and then do a single copy / move. memmove would work, as that allows overlaps between source and destination, or you can just loop through the characters yourself to copy. — Rup, Jan 07 '20 at 13:55
@Rup [cplusplus.com is not a reliable reference](https://stackoverflow.com/questions/6520052/whats-wrong-with-cplusplus-com), and it's certainly not official "documentation" of any sort. — Andrew Henle, Jan 07 '20 at 13:58
@AndrewHenle It's right in this instance though :-p Perhaps I should have linked to a glibc man page instead. — Rup, Jan 07 '20 at 14:01
@Rup No, actually it's not right. Not at all. "Shall not overlap" is not the same as "the behavior is undefined". — Andrew Henle, Jan 07 '20 at 14:05
Sidenote: You should **really** abandon those ancient K&R style parameter list you use in `main`. And `main` should usually have return type `int` — Gerhardh, Jan 07 '20 at 14:09
@AndrewHenle this might be a bit pedantic, but it is "should not", not "shall not" — Gerhardh, Jan 07 '20 at 14:11
After reading the question again, the result does not make any sense. Are you sure, you read "CHEDDAR" from file? Or do you have some leading space? While your code is broken, it should not touch the string at all if you don't have a space. — Gerhardh, Jan 07 '20 at 14:15
Do you still get same problem if you change to `char result[] = "CHEDDAR";` ? — Gerhardh, Jan 07 '20 at 14:18

score 4 · Accepted Answer · edited Jun 20 '20 at 09:12

4

You copy a string using overlapping memory area:

strcpy(startingString, startingString + 1);

From the C standard:

7.24.2.3 The strcpy function

If copying takes place between objects that overlap, the behavior is undefined.

You need to use memmov (and provide proper length) or you need to move the characters on your own. You can also improve the performance if you start with counting the characters that need to be removed and then copy all in one go.

Another issue that was pointed out by Joop Eggen in a comment:

char *result = "CHEDDAR";

You are not allowed to modify string literals. If you try to remove leading characters, you invoke undefined behaviour.

You should change this to

char result[] = "CHEDDAR";

As your sample string does not contain a leading space, this does not cause trouble yet. But you should fix it nevertheless

edited Jun 20 '20 at 09:12

Community

1
1

answered Jan 07 '20 at 13:56

Gerhardh

11,688
4
17
39

Thanks, but how does it even enter the while loop when there are no leading spaces? – Kyle Jan 07 '20 at 14:16
That is what I meant with my latest comment under your question. Are you really sure that you don't read spaces from the file? – Gerhardh Jan 07 '20 at 14:17
ah yes I missed it, sorry – Kyle Jan 07 '20 at 14:32

score 3 · Answer 2 · answered Jan 07 '20 at 13:56

This code

strcpy(startingString, startingString + 1);

copies overlapping strings.

Per 7.24.2.3 The strcpy function, paragraph 2 of the C standard:

The strcpy function copies the string pointed to by s2 (including the terminating null character) into the array pointed to by s1. If copying takes place between objects that overlap, the behavior is undefined.

You are invoking undefined behavior.

Paul Ogilvie · Answer 3 · 2020-01-07T14:05:47.537

Although the answers identifying that you are copying overlapping strings identify undefined behavior, another cause is that you use a literal string, and on most platforms those are immutable and will cause the program to abort.

Instead of:

char *result = "CHEDDAR";

Use:

char result[] = "CHEDDAR";

(Note: looking at how most strcpy functions will have been implemented, namely a loop that terminates when seeing the null character of the source string, then the overlap that you use will still see the null character of the source and place it in the destination (down-copying). Copying the other way around (up-copying) would not see the null terminator anymore, as it will have been overwritten, and may continue copying beyond the destination string.)

score 2 · Answer 4 · answered Jan 07 '20 at 14:34

2

In your case, where no modification is needed and no allocs are done, you only need to find the start without copying anything.

char *stripFrontChar(char *startingString, char removeChar) {
    for( ; *startingString == removeChar; startingString++)
        ;
    return (startingString);
}

But you have to use the return of stripFrontChar()

printf("%s\n", stripFrontChar(result));

answered Jan 07 '20 at 14:34

Holger

899
2
7
12

Better solution than strcpy or memmov. – stark Jan 07 '20 at 15:16

C code removing wrong character from string

4 Answers4