Segment Fault 11 with Dynamic C-String being Displayed from Class Function

Question

Dynamic c-string 'textString' causes Segmentation fault 11 when used by a class member function to be displayed.

#include "Text.h"
#include <iostream>
#include <cstring>
using namespace std;

Text::Text(const char* textArray)
    {
      textLength = strlen(textArray);
      char* textString = new char[textLength];
      strcpy(textString, textArray);
      cout << textString << endl;
    }

Text::~Text()
{
  delete[] textString;
}

void Text::displayText() const
{
  cout << textString << endl;
}

My provided driver:

#include <iostream>
#include "Text.h"

using namespace std;

int main()
{
    Text *myString1;
    char tvshow[1000];

    cout << "\n\nWhat is the most recent TV show you watched?\n";
        cin.getline(tvshow, 1000);
        myString1 = new Text(tvshow);

    cout << "\n\nTV SHOW:\t";
        myString1->displayText();
        cout << "\t# CHARACTERS:\t" << myString1->getLength() << endl;

    return 0;
}

In the Constructor for the Text class, the line cout << textString << endl; works as intended. However, when the main function calls myString1->displayText();, bash ends the program with Segmentation fault: 11.

Any help would be greatly appreciated. Thank you.

Save yourself a lot of time, effort and frustration and use `std::string` instead. — NathanOliver, Sep 11 '18 at 13:53
I agree, but unfortunately my current class is forcing me to use c-strings for now. — chasehaley, Sep 11 '18 at 13:54
In what way is it forcing the use of c-strings that `std::string::c_str()` doesn't alleviate? — Max Langhof, Sep 11 '18 at 13:58
@MaxLanghof Sure, VLAs are non-standard C++, however, I fail to see VLAs being used here. — Algirdas Preidžius, Sep 11 '18 at 14:05
Please learn how to create a [MCVE](https://stackoverflow.com/help/mcve) because it helps people to get better into your problem and reproduce it faster. See [this](https://wandbox.org/permlink/uJniD4G1ihK1Mkrl) as an example. — hellow, Sep 11 '18 at 14:17
@hellow Thank you for the example. I'll try to make my posts easier to read next time. — chasehaley, Sep 11 '18 at 14:35

score 3 · Answer 1 · edited Sep 11 '18 at 14:07

3

You are witnessing undefined behavior, due to strcpy(textString, textArray); writing out of bounds.

In

Text::Text(const char* textArray)
{
    textLength = strlen(textArray);
    char* textString = new char[textLength];
    strcpy(textString, textArray);
    cout << textString << endl;
}

strlen (textArray) returns the length of a null-terminated string pointed at by textArray. Such length doesn't include the null-terminator itself. However, strcpy writes the null-terminator to the destination string. To avoid such an issue, allocate a place for the null-terminator:

Text::Text(const char* textArray)
{
    textLength = strlen(textArray);
    char* textString = new char[textLength + 1];
    strcpy(textString, textArray);
    cout << textString << endl;
}

edited Sep 11 '18 at 14:07

hellow

12,430
7
56
79

answered Sep 11 '18 at 13:59

Algirdas Preidžius

1,769
3
14
17

I've made the change you suggested with `char* textString = new char[textLength + 1]` but the error still persists. I'm also still unsure how the cout statement works fine in the constructor but causes a Segmentation Fault in the displayText() function – chasehaley Sep 11 '18 at 14:08
@chasehaley Then follow the answer of Hellow. I missed the fact that you are not assigning to the class member. :/ – Algirdas Preidžius Sep 11 '18 at 14:10

hellow · Accepted Answer · 2018-09-11T14:10:49.910

Additionaly to what Algirdas says, your other problem is, that you actually don't assign the class member textString (assuming you have one), but shadow it in your constructor. Change your constructor to something like this:

Text::Text(const char* textArray)
{
    textLength = strlen(textArray);
    textString = new char[textLength + 1];
    strcpy(textString, textArray);
    cout << textString << endl;
}

Note the missing char * in front of textString.

How did I spot this?

As with any other segfault, you should learn how to debug. Your best tools are gdb and valgrind. Running this with valgrind gives me:

$ echo franz | valgrind ./a.out
==6222== Memcheck, a memory error detector
==6222== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6222== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==6222== Command: ./a.out
==6222== 


What is the most recent TV show you watched?
franz


TV SHOW:    ==6222== Invalid read of size 1
==6222==    at 0x4C32CF2: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6222==    by 0x108DB6: Text::getLength() const (a.cpp:37)
==6222==    by 0x108E70: main (a.cpp:51)
==6222==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==6222== 
==6222== 
==6222== Process terminating with default action of signal 11 (SIGSEGV)
==6222==  Access not within mapped region at address 0x0
==6222==    at 0x4C32CF2: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6222==    by 0x108DB6: Text::getLength() const (a.cpp:37)
==6222==    by 0x108E70: main (a.cpp:51)
==6222==  If you believe this happened as a result of a stack
==6222==  overflow in your program's main thread (unlikely but
==6222==  possible), you can try to increase the size of the
==6222==  main thread stack using the --main-stacksize= flag.
==6222==  The main thread stack size used in this run was 8388608.
==6222== 
==6222== HEAP SUMMARY:
==6222==     in use at exit: 14 bytes in 2 blocks
==6222==   total heap usage: 5 allocs, 3 frees, 77,838 bytes allocated
==6222== 
==6222== LEAK SUMMARY:
==6222==    definitely lost: 6 bytes in 1 blocks
==6222==    indirectly lost: 0 bytes in 0 blocks
==6222==      possibly lost: 0 bytes in 0 blocks
==6222==    still reachable: 8 bytes in 1 blocks
==6222==         suppressed: 0 bytes in 0 blocks
==6222== Rerun with --leak-check=full to see details of leaked memory
==6222== 
==6222== For counts of detected and suppressed errors, rerun with: -v
==6222== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
fish: Process 6222, “valgrind” “echo franz | valgrind ./a.out” terminated by signal SIGSEGV (Address boundary error)

You can clearly see, that the crash happens in getLength, because it tries to access address 0x0. When you run this with gdb, you will stop at exactly that point and can see print textString, that it is 0 and not properly initiliazed.

Please read What is a debugger and how can it help me diagnose problems? for further information!

"_because it tries to access address 0x0._" Technically, since the `textString` is uninitialized in this case, the address itself can be equal to anything (contain indeterminate value). — Algirdas Preidžius, Sep 11 '18 at 14:12
@AlgirdasPreidžius I set it to `nullptr` in my example, because it was not a MCVE. Maybe I spotted it because of that. — hellow, Sep 11 '18 at 14:15
@hellow Well, I was commenting based on the code shown, which is, just a technicality, in my eyes. — Algirdas Preidžius, Sep 11 '18 at 14:21

score 0 · Answer 3 · answered Sep 11 '18 at 14:10

I am assuming that textLengthand textString are member variables of your class (of type size_t and char * respectively). You don't show the definition of your class Text.

You made a mistake in your constructor. This line

char* textString = new char[textLength];

should be

char* textString = new char[textLength+1];

to allow for the trailing '\0' that C strings have. Also you might want to bulletproof your class Text by making sure your write a copy contructor, destructor and assignment operator. Also consider what happens if NULL is passed to Text constructor. Alternatively a better option is to use std::string as a member variable, which means you can do away with the copy constructor, destructor and assignment operator (the compiler generated versions will just work); you can also do away with textLength and you can write a constructor that is member initialised.

Segment Fault 11 with Dynamic C-String being Displayed from Class Function

3 Answers3

How did I spot this?