splitting char *array using strtok giving runtime error

Question

My code is pretty simple(just splitting an array) but since I don't have a c compiler, I'm compiling it online on http://ideone.com. However, it gives runtime error no matter what and I've also tried parsing with sscanf but in vain. Please help.

#include<stdio.h>
#include<string.h>
typedef struct{
   char *header;
   char *body;
}AT_msg_Data;
static char *__rx_data = "AT+CGMI\r";
int main(){
    AT_msg_Data *data;
    printf("In main");
    data->header = strtok(__rx_data,"+");
    data->body = strtok(NULL,"+");
    //also tried sscanf_s(data->body,"[^=]%s\r",at_comm,sizeof(at_comm),at_param,sizeof(at_param));
    printf("%s %s", data->header, data->body);
}

Try `static char *__rx_data = "AT+CGMI\r";` --> `static char __rx_data[] = "AT+CGMI\r";` — Spikatrix, Sep 29 '15 at 12:42
static char *__rx_data = "AT+CGMI\r"; // this is read only data try @CoolGuy 's approach — asio_guy, Sep 29 '15 at 12:54
possible duplicate of [Strtok segfault](http://stackoverflow.com/questions/7100214/strtok-segfault) — AndersK, Sep 29 '15 at 13:10
so many strdup questions so little interest in looking if the question was previously posted — AndersK, Sep 29 '15 at 13:11

Iharob Al Asimi · Answer 1 · 2015-09-29T13:09:11.063

You can't call strtok() on a string literal, because string literals are constant. Generally it's safer to declare pointers to string literals with the const qualifier, to ensure the compiler will warn you when the program tries to modify the pointee.

You need to make your string an array instead of a pointer to a string literal, like this

static char __rx_data[] = "AT+CGMI\r";

this is an array of 9 bytes containing a string of 8 characters which you can modify, strtok() does modify it's arguments.

You also need to allocate space for your struct to be able to use it, you can do it in two ways, using malloc()

AT_msg_Data *data;
data = malloc(sizeof(*data));
if (data == NULL)
    return -1; /* failed to allocate memory -- exit the program */

or, just create an instance on the stack

AT_msg_Data data;
/*         ^ no star here */

and then access it's members with a . instead of ->.

Another way which does not apply in this case because your string cannot be allocated dynamically, is to use malloc() or strdup().

You could also use strchr() like this

const char *plus;
plus = strchr(__rx_data, '+');
if (plus != NULL)
{
    size_t length; /* maybe use `ptrdiff_t' here but just for illustration */
    length = plus - __rx_data;
    data->header = malloc(length + 1); 
    if (data->header != NULL)
    {
        memcpy(data->header, __rx_data, length);
        data->header[length] = '\0';
        data->body = strdup(plus + 1);
    }
    /* note: data->header can be `NULL' from here, so check that before using it */
    /*       this also applies to data->body */
}

This way you will not need to modify __rx_data which is better because it's a global variable and I assume you are going to reuse it. You should make your code safer by then redeclaring

static const char *const __rx_data = "AT+CGMI\r";

if you are going to use the strchr() approach.

With this declaration __rx_data cannot be reassigned or modified without the compiler complaining about it.

Your code will anyway fail later, because the '\0' placed by strtok() at the position of the + sign will be overwritten with the + once you call strtok() again, strtok() keeps an internal buffer to restore the replaced characters that also makes it unsafe if your program is multithreaded.

Just to notice, his line `AT_msg_Data *data;` doesn't allocate memory for his structure to use. Could you at this to your answer, just for correctness pls. — zeyorama, Sep 29 '15 at 12:49
@zeyorama I didn't even see that, when I saw `static char *__rx_data = "AT+CGMI\r"` I immediately started writing the answer since `strtok()` + *string literal* immediately triggered something in my brain. — Iharob Al Asimi, Sep 29 '15 at 12:57
@iharob Sorry , I got bit rude (that comment sounded like you upvoted just because I argued .I didn't argued for that . What you said was your opinion and I had mine just that ). Sorry again !! :) — ameyCU, Sep 29 '15 at 13:36

ameyCU · Answer 2 · 2015-09-29T14:05:03.307

1

You can use sscanf but first you need to allocate memory for data , header and body.

data = malloc(sizeof(AT_msg_Data));
if(data==NULL){
      // handle error
 }
data->header = malloc(10);
if(data->header==NULL){
      // handle error
 }

data->body = malloc(20);
if(data->body==NULL){
      // handle error
 }

And then use sscanf like this -

 if(sscanf(__rx_data,"%[^+]+%[^\r]\r",data->header,data->body)==2){     // if sscanf is successful 
   //do something
  }

Then after this data->header will conatain AT and data->body will have CGMI from the string.

edited Sep 29 '15 at 14:05

answered Sep 29 '15 at 12:58

ameyCU

16,489
2
26
41

This as it is, will probably invoke undefined behavior. It's a smart solution, but bear in mind you are giving this solution to a beginer, there is a huge chance of misusing `sscanf()` ignoring the return value of `malloc()` and thus posting another question *my program is crashing* or my neighbor is eating their dog. – Iharob Al Asimi Sep 29 '15 at 12:59
Well if `sscanf()` `data->header` or `data->body` or bothe might be read without initialization, and also `malloc()` could return `NULL`. And alos, never `malloc()` for a constant value unless extremely necessary. Instead of `malloc(10)` or ... You could just redefine them in the `struct` definition like `char header[10]`. And always `malloc()` the exact ammount, there no infinite memroy. – Iharob Al Asimi Sep 29 '15 at 13:01
@iharob He is using pointers . At some or other point he will have to allocate memory . I agree on `malloc(10)` part . But what if need it to be done with pointers only ? – ameyCU Sep 29 '15 at 13:05
@iharob I just posted this as he said he tried `sscanf` also ,so assuming he would know about it . – ameyCU Sep 29 '15 at 13:09

splitting char *array using strtok giving runtime error

2 Answers2