Segmentation fault in constructor when using strcpy

Question

Hi guys I'm writing a program that reads NMEA sentences for a university assignment and I'm having trouble with segmentation fault. Can anyone help me fix it, please?

NmeaSentence::NmeaSentence(std::string sentence) {
    const char *temp = sentence.c_str();
    char *c_sent;
    strcpy(c_sent, temp);
    char *pch;
    for(int i = 0; i < MAX_SENTENCE_PARTS; i++){
        pch = strtok(c_sent, ",");
        this->sentenceParts[i] = pch;
    }
    this->sentence = sentence;
    this->sentenceType = sentenceParts[0];
}

The error seems to be happening at strcpy. What am I doing wrong?

possible duplicate of [strcpy with malloc?](http://stackoverflow.com/questions/5354933/strcpy-with-malloc) — Shafik Yaghmour, Mar 04 '14 at 15:33
On a side note, why not using `std::string` everywhere? Is there a specific need that forces you to manipulate strings through `(const) char *`? — JBL, Mar 04 '14 at 15:35
You're using `strtok`. That's plenty wrong. Consider using Boost.Tokenizer, or `split` from Boost.StringAlgo. — Sebastian Redl, Mar 04 '14 at 15:50

Bathsheba · Accepted Answer · 2014-03-04T15:40:27.003

2

You don't allocate memory for c_sent. That's undefined behaviour.

Use char *c_sent = new char[sentence.size() + 1];. I've added space for the null terminator. Don't forget to call delete[] c_sent; before the function exits.

(By the way, temp is valid for the lifetime of sentence, unless it's modified in any way.).

edited Mar 04 '14 at 15:40

answered Mar 04 '14 at 15:34

Bathsheba

231,907
34
361
483

Actually it is valid until sentence is modified, so it would be prudent to make sentence const. – heinrichj Mar 04 '14 at 15:36
I'm not entirely sure, I'm new to C++ – Will Mar 04 '14 at 15:36
@heinrichj: that's a very good point. Better still, pass `const std::string& sentence` as the function parameter. – Bathsheba Mar 04 '14 at 15:38
@Will If you're new to C++, the best advice I can give you is to use `std::string` as much as possible. Drop all the old `char*` stuff. – Bathsheba Mar 04 '14 at 15:39

score 0 · Answer 2 · answered Mar 04 '14 at 15:39

0

The temporary string c_sent is uninitialized.

char * c_sent

to

char * c_sent = strdup(sentence.c_str());

Dont forget to free, before exit.

free(c_sent);

You won't need temp this way.

answered Mar 04 '14 at 15:39

Sanjaya R

6,246
2
17
19

1

We're in C++ here, I'd advise against using `free` etc. – Bathsheba Mar 04 '14 at 15:39
Well, if you are using strtok, you might as well use strdup and free. – Sanjaya R Mar 04 '14 at 15:42
@Bathsheba you must use `free` with `strdup`, although using `strdup` is [may not be](http://stackoverflow.com/questions/12984948/why-is-strdup-considered-to-be-evil) a great idea. – Shafik Yaghmour Mar 04 '14 at 15:45

score 0 · Answer 3 · answered Mar 04 '14 at 15:54

The member function has several defects.

If the parameter of the function is not changed then it would be better to declare the function as

NmeaSentence::NmeaSentence( const std::string & sentence);

As it was already said you did not allocate memory where you are going to copy sentence. Pointer c_sent was not initialized by the address of an allocated memory.

The second defect is that pch always points to the same address in c_sent because you are incorrectly using function strtok. You should use it the following way

char *pch = strtok(c_sent, ",");
for(int i = 0; i < MAX_SENTENCE_PARTS && pch; i++){
    this->sentenceParts[i] = pch;
    pch = strtok( NULL, ",");
}

Also it is not clear how you will determine how many parts the string contains.

Segmentation fault in constructor when using strcpy

3 Answers3