Substring garbage in C

Question

I want to make a program that cuts a string into 2 strings if the string contains a ?

For example,

Test1?Test2

should become

Test1 Test2

I am trying to do it dynamically but at the prints I get some garbage and I cannot figure out what I am doing wrong. Here is my code:

for(i=0;i<strlen(expression);i++){

        if(expression[i]=='?' || expression[i]=='*'){
            position=i; break;

        }
    }

    printf("position=%d\n",position);

    if(position!=0){
        before = (char*) malloc(sizeof(char)*(position +1 ));
       strncpy(before,expression,position);
       before[strlen(before)]='\0';

    }

    if(position!=strlen(expression))
    {
        after = (char*) malloc(sizeof(char)*( strlen(expression)- position+1));
        strncpy(after,expression+position+1,strlen(expression)-position+1);
        after[strlen(after)]='\0';

    }

    printf("before:%s,after:%s\n",before,after);

You're overcomplicating this. `char first[] = "first?second"; char *p = strchr(first, '?'); *p = 0; char *second = p + 1;` <- after this, `first` will contain the first part of the string, and `second` will point to the beginning of the second part. If you want to just *replace* the question mark with a space, then do `*p = ' ';` instead. Also, [don't cast the return value of `malloc()`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858#605858)! — , Jan 05 '14 at 20:03
You might want to look into e.g. [`strchr`](http://en.cppreference.com/w/c/string/byte/strchr). — Some programmer dude, Jan 05 '14 at 20:05

score 3 · Answer 1 · answered Jan 05 '14 at 20:04

3

Since it is illegal to take length of a string until it is null terminated, you cannot do this:

before[strlen(before)]='\0';

You need to do this instead:

before[pos]='\0';

Same goes for the after part - this is illegal:

after[strlen(after)]='\0';

You need to compute the length of after upfront, and then terminate the string using the pre-computed length.

answered Jan 05 '14 at 20:04

Sergey Kalinichenko

714,442
84
1,110
1,523

Ok! Thx for advide I will tag the answer as soon as possible! – JmRag Jan 05 '14 at 20:07
Another problem is that position is not initialized if no separator character is found - so probably he'll get a segfault at the second strncpy(). – ldx Jan 05 '14 at 20:08
@ldx I am not sure about that, because there is no declaration of `position` shown in the code snippet. – Sergey Kalinichenko Jan 05 '14 at 20:09

score 1 · Answer 2 · edited May 23 '17 at 11:56

  for(i=0;i<strlen(expression);i++){

This is very inefficient. strlen(expression) gets computed at every loop (so you get an O(n²) complexity, unless the compiler is clever enough to prove that expression stays a constant string and then to move the computation of strlen(expression) before the loop.....). Should be

  for (i=0; expression[i]; i++) {

(since expression[i] is zero only when you reached the terminating null byte; for readability reasons you could have coded that condition expression[i] != '\0' if you wanted to)

And as dasblinkenlight answered you should not compute the strlen on an uninitialized buffer.

You should compute once and for all the strlen(expression) i.e. start with:

size_t exprlen = strlen(expression);

and use exprlen everywhere else instead of strlen(expression).

At last, use strdup(3):

 after = (char*) malloc(sizeof(char)*( strlen(expression)- position+1));
 strncpy(after,expression+position+1,strlen(expression)-position+1);
 after[strlen(after)]='\0';

should be

 after = strdup(expression-position+1);

and you forgot a very important thing: always test against failure of memory allocation (and of other system functions); so

    before = (char*) malloc(sizeof(char)*(position +1));

should really be (since sizeof(char) is always 1):

    before = malloc(position+1);
    if (!before) { perror("malloc of before"); exit(EXIT_FAILURE); };

Also, if the string pointed by expression is not used elsewhere you could simply clear the char at the found position (so it becomes a string terminator) like

    if (position>=0) expression[position] = `\0`;

and set before=expression; after=expression+position+1;

At last, learn more about strtok(3), strchr(3), strstr(3), strpbrk(3), strsep(3), sscanf(3)

BTW, you should always enable all warnings and debugging info in the compiler (e.g. compile with gcc -Wall -g). Then use the debugger (like gdb) to step by step and understand what is happening... If a memory leak detector like valgrind is available, use it.

@Mario - `expression[i]` will return true when there is value. on null terminated string, it will return false as soon as you hit the null terminator. — alvits, Jan 05 '14 at 20:09
@alvits Thx.. Withought your explanation I woyldn't understand it. — JmRag, Jan 05 '14 at 20:12

score 1 · Answer 3 · answered Jan 05 '14 at 20:05

1

You can use strsep

char* token;

while ((token = strsep(&string, "?")) != NULL)
  {
    printf("%s\n", token);
  }

answered Jan 05 '14 at 20:05

laaposto

11,835
15
54
71

2

(delimiters are `?` and `*` for the OP) – ShinTakezou Jan 05 '14 at 20:07

score 1 · Answer 4 · answered Jan 05 '14 at 20:11

1

Instead of doing it manually, you can also make good use of strtok(). Check the details here.

answered Jan 05 '14 at 20:11

Sourav Ghosh

133,132
16
183
261

Substring garbage in C

4 Answers4