Strtok isn't returning as expected, am I using it wrong?

Question

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

// fiter string to the first |
char* filterstringfirst(char* command, int i){
    char *tok = command;
    int x = 0;
    while ((tok = strtok(tok, "|")) != NULL && x <= i)
    {
        if( x == i){
            return tok;
        }
        x++;
        printf(" ===  Parsed: --%s-- ===\n", tok);
        tok = NULL;
    }
    return tok;
        
}


int main () {
    char command[]  = "ls -a | sort -h | grep h | wc -l";
    
    char command2[]  = "ls -a | sort -h | grep h | wc -l";
    char* temp = command;

    char* x =  filterstringfirst(temp, 0);
    printf("%s\n",x);
    char* temp2 = command;

    char* x2 =  filterstringfirst(temp2, 1);
    printf("%s\n",x2);
    
    
   
    temp = command;
    

   return 0;
}

I have this function I made which is supposed to just return part of a string. The original string should be similar to "ls -l | grep temp | sort".

The idea was that it would be called with the string and a number, and return that segment. Eg. 0 -> "ls -l"

Now this works the first time I call it, but calling it again seems to break and end in a segfault.

char command[]  = "ls -a | sort -h | grep h | wc -l";
char* temp = command;

char* x =  filterstringfirst(temp, 0);
printf("%s\n",x);
char* temp2 = command;

char* x2 =  filterstringfirst(temp2, 1);
printf("%s\n",x2);`

This was my testing code

And the output:

ls -a  
===  Parsed: --ls -a -- === 
[1]    1126 segmentation fault  ./templ
 ➜  Current gcc -o templ templ.c
 ➜  Current ./templ ls -a  
===  Parsed: --ls -a -- === [1]   
 1136 segmentation fault  ./templ

Edit: Updated to have main too (based on comments)

@WeatherVane I was going to say the same thing, but then I saw `tok = NULL;` at the bottom of the loop. — Barmar, Mar 28 '22 at 19:27
Your code would be easier to read for yourself and other people if you used [indentation](https://en.wikipedia.org/wiki/Indentation_style). Edit: Meanwhile, someone else has added the indentation for you. — Andreas Wenzel, Mar 28 '22 at 19:28
@Barmar mybad: not sharp enough to see past the (lack of) formatting. — Weather Vane, Mar 28 '22 at 19:28
The second call to `filterstringfirst` won't work, because `strtok()` modifies the string. — Barmar, Mar 28 '22 at 19:29
Why do you need the `temp` and `temp2` variables? You can just use `filterstringfirst(command, ...)` — Barmar, Mar 28 '22 at 19:31
The check of `tok` here is also unnecessary: `if( x == i && tok != NULL)` because you won't be in the loop if `tok == NULL`. — Weather Vane, Mar 28 '22 at 19:35
I noticed it isn't `const char[]`. How attached are you to those strings? — Neil, Mar 28 '22 at 22:43

Vlad from Moscow · Answer 1 · 2022-03-28T20:50:33.933

The function strtok changes the passed string by inserting zero characters '\0' in the positions of delimiters.

So after the first call of the function filterstringfirst

char* x =  filterstringfirst(temp, 0);

the character array command looks like

"ls -a \0 sort -h | grep h | wc -l";
       ^^^

That is in fact you have the following string "ls -a " stored in the array command.

So calling the function the second time with the second argument greater than 0 you will get as a result a null pointer.

If you want to extract substrings specifying an index then you should use functions strspn and strcspn and return from the function a dynamically allocated array containing the target substring.

Here is a demonstration program that shows how the function can be defined using the standard string functions strspn and strcspn and without creating dynamically a copy of the source string (that is inefficient and unsafe) each time when the function is called.

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

char * filterstringfirst( const char *command, const char *delimiters, size_t i ) 
{
    char *substring = NULL;

    const char *p = command;
    size_t n = 0;

    do
    {
        p += n;
        p += strspn( p, delimiters );
        n = strcspn( p, delimiters );
    } while (*p && i--);

    if ( *p && ( substring = malloc( n + 1 ) ) != NULL )
    {
        memcpy( substring, p, n );
        substring[n] = '\0';
    }

    return substring;
}

int main( void )
{
    char command[] = "ls -a | sort -h | grep h | wc -l";
    const char *delimiters = "|";

    char *substring = NULL;

    for (size_t i = 0;
        ( substring = filterstringfirst( command, delimiters, i ) ) != NULL;
        i++)
    {
        printf( "%zu: \"%s\"\n", i, substring );
        free( substring );
    }
}

The program output is

0: "ls -a "
1: " sort -h "
2: " grep h "
3: " wc -l"

You can use this function with any delimiters used to separate a string.

Ah, that makes sense! I'm assuming the best way to go about this would be a loop and pointers then? Is there a better way I'm missing? — DynamicApproach, Mar 28 '22 at 19:46
@DynamicApproach You could call the function without the second parameter sequentially passing NULL to it after the first call. — Vlad from Moscow, Mar 28 '22 at 19:49
removing the second param, so no int? So calling it once gets the "ls -a" Then calling it again gets the "sort -h"? Am I correct in that thinking? Eg. `char* filterstringfirst(char* command){ char *tok = command; int x = 0; while ((tok = strtok(tok, "|")) != NULL ) { x++; printf(" === Parsed: --%s-- ===\n", tok); tok = NULL; } return tok; }` — DynamicApproach, Mar 28 '22 at 20:00
@DynamicApproach The variable x is redundant. You should track the number of a substring in the caller of the function. — Vlad from Moscow, Mar 28 '22 at 20:05

Oka · Accepted Answer · 2022-03-28T20:01:14.157

strtok is destructive - it modifies the buffer passed in by replacing delimiters will null bytes.

After

char* x =  filterstringfirst(temp, 0);

command will effectively be "ls -a ".

If you want to use strtok here, you will either need to:

mimic strtok in your wrapping function, by passing NULL and the position to start from in subsequent calls, or
duplicate the string before using it, and return a copy of the token.

An example of the second, with no error handling:

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

char *get_token_at(char *command, size_t n) {
    size_t position = 0;
    char *copy = strdup(command);
    char *token = strtok(copy, "|");
    char *output = NULL;

    while (token && position < n) {
        token = strtok(NULL, "|");
        position++;
    }

    if (token && position == n)
        output = strdup(token);

    free(copy);

    return output;
}

int main(void) {
    char command[]  = "ls -a | sort -h | grep h | wc -l";

    char *x = get_token_at(command, 0);
    puts(x);
    free(x);

    x = get_token_at(command, 1);
    puts(x);
    free(x);
}

stdout:

ls -a 
 sort -h

(Note the whitespace in these tokens.)

Strtok isn't returning as expected, am I using it wrong?

2 Answers2