segmentation fault on some strtok calls

Question

This is a function running inside a loop. Basically the app receives a message from Arduino and sends the message to this function:

void parseMessage(char *message){

    char *sensor_num="";
    char *value="";
    char *percentage="";
    char *delimiter = "|";

    sensor_num = strtok(message , delimiter);
    value = strtok(NULL, delimiter);
    percentage = strtok(NULL, delimiter);

    // Do stuff with values
}

It keeps running for a time, but sometimes I get a segfault on the value = strtok(NULL, delimiter); part.

The message arriving is like:

1|123|50|

And the main function that is calling parseMessage

int main() {

  SYS_LOG_INFO("Opening device... ");
  int BLUETOOTH = open("/dev/tty.HC-05-DevB", O_RDONLY | O_NOCTTY | O_NONBLOCK);
  SYS_LOG_INFO("Opened!");

  struct termios tty;
  struct termios tty_old;
  memset (&tty, 0, sizeof tty);

  /* Error Handling */
  if (tcgetattr(BLUETOOTH, &tty) != 0) {
    SYS_LOG_ERROR("Error on read Bluetooth.");
    return 1;
  }

  /* Save old tty parameters */
  tty_old = tty;

  /* Set Baud Rate */
  cfsetospeed (&tty, (speed_t)B9600);
  cfsetispeed (&tty, (speed_t)B9600);

  /* Setting other Port Stuff */
  tty.c_cflag     &=  ~PARENB;        // Make 8n1
  tty.c_cflag     &=  ~CSTOPB;
  tty.c_cflag     &=  ~CSIZE;
  tty.c_cflag     |=  CS8;

  tty.c_cflag     &=  ~CRTSCTS;       // no flow control
  tty.c_cc[VMIN]  =   1;
  tty.c_cc[VTIME] =   5;
  tty.c_cflag     |=  CREAD | CLOCAL; // turn on READ & ignore ctrl lines

  /* Make raw */
  cfmakeraw(&tty);

  /* Flush Port, then applies attributes */
  tcflush(BLUETOOTH, TCIFLUSH);
  if ( tcsetattr ( BLUETOOTH, TCSANOW, &tty ) != 0) {
    SYS_LOG_ERROR("Error on flush port.");
    return 1;
  }

  int n = 0;
  char buf[255];

  SYS_LOG_INFO("Starting to read data...");
  do {
    n = read(BLUETOOTH, &buf, sizeof buf);

    if (n > 0) {
      printf("buf = %s\n", buf);
      parseMessage(buf);
      memset(&buf, '\0', sizeof buf);
    }

    usleep(500000);  /* sleep for 100 milliSeconds */
  } while(1);

  return 0;
}

The problem is most likely not with your function, but with the data fed to it. — StoryTeller - Unslander Monica, Mar 03 '17 at 23:29
Please check the return value from `strtok` before asking it again. `if(sensor_num != NULL) { value = strtok(NULL, delimiter); }` ditto the next item. With data fed to you: always check, never assume it is right. — Weather Vane, Mar 03 '17 at 23:31
@Weather Vane: right concerning the check of the data feed. But "not checking return value of `strtok`" does not explain the crash, since `strtok` will return another `NULL` if it has once reached the end, right? — Stephan Lechner, Mar 03 '17 at 23:35
@StephanLechner seems so, I just tested repeatedly calling `strtok` after `NULL` was returned, but perhaps the example code shown is incomplete, and the actual code is attempting to *process* the `NULL` pointers from `strtok`. — Weather Vane, Mar 03 '17 at 23:39
Gilbert, please provide the code calling `parseMessage`; maybe you pass a string which lacks a string terminating character or lets `strtok` somehow else introduce it's token separating `'\0'`-character at a position where this is not allowed. — Stephan Lechner, Mar 03 '17 at 23:44
There is also undefined behavior if the `message` being passed in is a string literal. http://stackoverflow.com/questions/8957829/strtok-segmentation-fault?rq=1 — Peter, Mar 03 '17 at 23:46
@StephanLechner I've updated my question with the main function that calls the parser. Many thanks! — Gilbert, Mar 04 '17 at 00:00
If your `read` fills up `buf` solely with values `> '\0'`, then `buf` will not be a "string" in the sense of terminated by a `\0` and `strtok` will consequently write to a memory location outside the boundaries of `buf`. Can you check this? — Stephan Lechner, Mar 04 '17 at 00:06
@Gilbert, you might want to replace `memset(&buf, '\0', sizeof buf);` with `memset(buf, '\0', sizeof buf);` and try again. You probably want to reset the array, not the address of the address of the first element of the array. — VHS, Mar 04 '17 at 00:26
@VHS: `&buf` is not "address of the address of the first element of the array". `&buf` is address of the entire array, which is guaranteed to be the same memory location as `&buf[0]`. This means that `memset(&buf..` is equivalent to `memset(buf...`. After conversion to `void *` the same value will be passed to `memset` in either case. One can even argue that `memset(&buf...` is a more appropriate, elegant and logical way to do it if you use `sizeof buf` as the size. — AnT stands with Russia, Mar 04 '17 at 01:48
On the loop's 1st iteration the buf is uninitialised: Move the `memset` before the `read`. Or do `char buf[255] = "";` — alk, Mar 04 '17 at 08:20
"*`usleep(500000); /* sleep for 100 milliSeconds */`*" hmm ...? — alk, Mar 04 '17 at 08:22

zzn · Answer 1 · 2017-03-04T01:52:51.830

0

1.As strtok is called inside parseMessage, buf should be nul-terminate, may get segmentation fault if not. try code below:

char buf[255]={0};

SYS_LOG_INFO("Starting to read data...");
n = read(BLUETOOTH, &buf, sizeof(buf)-1);

if (n > 0) {
  buf[n]=0;
  printf("buf = %s\n", buf);
  parseMessage(buf);
}

2. strtok is not reentrant, use strtok_r instead. sometimes, this may cause strange problems.

edited Mar 04 '17 at 01:52

answered Mar 04 '17 at 01:41

zzn

2,376
16
30

score 0 · Accepted Answer · answered Mar 04 '17 at 15:48

0

I am receiving some inconsistent data from Arduino.

So,I solved the problem checking the data with regex before send it to parse.

Thank you guys

answered Mar 04 '17 at 15:48

Gilbert

443
1
4
11

segmentation fault on some strtok calls

2 Answers2