strtok_r results in segmentation fault

Question

This code is to parse a csv file, but it results in segmentation fault. I could see the similar code here: Nested strtok function problem in C

They look same but my code results in segmentation fault. Why?

#include <cstdlib>
#include <string.h>
#include <stdio.h>
#include <vector>

using namespace std;

struct inputTuple{
    int user, item, rating;
};

int main(void)
{
    char xdata[] = "1,88,0,874965758;1,2,1,876893171;1,99,1,878542960;";
    vector<inputTuple> input;

            int maxUser = 0, maxItem = 0;
        int user, item, rating;

        char *end_str;
        char *data_point = strtok_r(xdata, ";", &end_str);

        while(data_point != NULL) {
            char *end_attr;
            char *data_point_attr = strtok_r(data_point, ",", &end_attr);
            while(data_point_attr != NULL) {

                user = atoi(data_point_attr);
                data_point_attr = strtok_r(NULL, ",", &end_attr);
                item = atoi(data_point_attr);
                data_point_attr = strtok_r(NULL, ",", &end_attr);
                rating = atoi(data_point_attr);
                strtok_r(NULL, ",", &end_attr);
                input.push_back({user, item, rating});
                maxUser = max(maxUser, user);
                maxItem = max(maxItem, item);
                }
            data_point = strtok_r(NULL, ";", &end_str);
        }
    return 0;
}

Don't use `strtok()` in c++. And not necessarily it's `strtok()`'s fault. — Iharob Al Asimi, Feb 09 '18 at 14:32
if a line doesn't have all the fields `data_point_attr = strtok_r(NULL, ",", &end_attr);` returns `NULL` and `atoi` triggers a segfault, which may be the case on the last token — Jean-François Fabre, Feb 09 '18 at 14:34
No, does not work even if I remove ; from last. http://coliru.stacked-crooked.com/a/dee251b2308b7cb9 — Kumar Roshan Mehta, Feb 09 '18 at 14:36
sorry cannot compile because my gcc doesn't have strtok_r... you really should use a C++ method to split strings... — Jean-François Fabre, Feb 09 '18 at 14:37
[`std::stringstream`](http://en.cppreference.com/w/cpp/io/basic_stringstream) is your friend here. Se how to use it [here](https://stackoverflow.com/questions/16446665/c-read-from-csv-file) — NathanOliver, Feb 09 '18 at 14:37
yeah: there are some NULL pointers here: check my assert fails at line 56 here: http://coliru.stacked-crooked.com/a/490160980cfa660f — Jean-François Fabre, Feb 09 '18 at 14:41

score 2 · Answer 1 · answered Feb 09 '18 at 14:37

2

Jean-Francois is correct. Just check the value before calling atoi like below. You need to do that every time you are calling atoi

if(data_point_attr != NULL)
    user = atoi(data_point_attr);

answered Feb 09 '18 at 14:37

Abhijit Pritam Dutta

5,521
2
11
17

ok, but the inner loop is the issue. Protecting against NULL will create one entry too much in the `vector` – Jean-François Fabre Feb 09 '18 at 14:52
1

@Jean-FrançoisFabre, yes you are right. I have up voted you :-) – Abhijit Pritam Dutta Feb 09 '18 at 14:54

score 2 · Accepted Answer · answered Feb 09 '18 at 14:50

your inner loop is not needed AND toxic to your program.

The inner loop extracts 3 tokens, then continues, extract the 4th token but reaches NULL in the middle, and atoi is passed NULL: segfault.

You only need one loop (added assert statements for sanity though):

    while(data_point != NULL) {
        char *end_attr;
        char *data_point_attr = strtok_r(data_point, ",", &end_attr);

            user = atoi(data_point_attr);
            data_point_attr = strtok_r(NULL, ",", &end_attr);
            assert(data_point_attr != NULL);
            item = atoi(data_point_attr);
            data_point_attr = strtok_r(NULL, ",", &end_attr);
            assert(data_point_attr != NULL);
            rating = atoi(data_point_attr);
            strtok_r(NULL, ",", &end_attr);
            input.push_back({user, item, rating});
            maxUser = max(maxUser, user);
            maxItem = max(maxItem, item);

        data_point = strtok_r(NULL, ";", &end_str);
    }

A better solution would be to use a real C++ solution to split the strings.

score 2 · Answer 3 · answered Feb 09 '18 at 14:54

According to Valgrind, your program is failing here:

                item = atoi(data_point_attr);

, as a result of the function argument being NULL. This seems to be consistent with Jean-François's prediction. A bit of debugging with gdb shows that it occurs on the second loop iteration, though you could have determined that even via print-statement debugging. And just a bit more debugging with gdb shows what's going on: your inner loop is faulty, because the loop condition does not become false when you reach the end of a record.

And what do you need that inner loop for, anyway? Nothing. You are reading each value via a separate statement, so you've effectively unrolled the loop vs. what one would need in a program to consume arbitrary CSV. Do check the result of each strtok_r() call, but drop the unneeded inner loop.

strtok_r results in segmentation fault

3 Answers3