C++ Segmentation Fault From Null Pointer Solution?

Question

I'm getting a segmentation fault on this program, and I know it has something to do with a null pointer being dereferenced, but I'm not exactly sure which one is causing the error. I'm just not certain as to how to fix the error while maintaining the purpose of the original program - it will compile, but at runtime I get the segfault I was just talking about.

main:

#include "link.h"
#include <iostream>
#include <string>
using namespace std;
int main()
{
 link * head_pointer = new link(NULL, NULL) ;
 for (int i = 0; i < 10; i++) {
 string new_string;
 getline(cin, new_string);
 string* pointer_to_input = new string(new_string);
 link * current_link = new link(head_pointer, pointer_to_input );
 head_pointer = current_link;
}

 head_pointer -> printAll(*head_pointer);
 return 42;
}

link:

#include <string>
#include <iostream>
#include "link.h"
using namespace std;
link::link(link * pointer_to_link, string * pointer_to_string)
{
  next = pointer_to_link;
  value = pointer_to_string;
}
link::~link() {
 delete value;
 delete next;
}
link * link::getNext() {
 return next;
}
string * link::getString() {
 return value;
}
int link::printAll(link link_to_print)  {
 cout << *link_to_print.getString() << endl;
 if (link_to_print.next != NULL) {
 return  printAll(*link_to_print.getNext());
} else {
 return 0;
}
}

Use fewer pointers, specifically less dynamic allocation. There's no need for all that. — GManNickG
– GManNickG, Commented Apr 29, 2017 at 3:57

Pavel P · Accepted Answer · 2017-04-29 04:28:25Z

2

Your destructor does look like an error, you shouldn't delete in destructor if you didn't allocate that in constructor:

link::~link() {
}

You should post your link.h to get more detailed explanation.

Without link.h it's not clear what else is wrong, however, there are also other problems:

link::printAll looks like a static method and should be called as: link::printAll(head_pointer);
you printAll should take by pointer, otherwise it it will create a copy of your link and delete it.
printAll has multiple issues as well. Probably it should have been something as follows:

void link::printAll(link *link_to_print)
{
    if (!link_to_print)
        return;
    if (link_to_print->getString())
        cout << *link_to_print->getString() << endl;
    printAll(link_to_print->next);
}

and your main:

int main()
{
    link * head_pointer = new link(NULL, NULL);
    for (int i = 0; i < 10; i++) {
        string new_string = str;
        getline(cin, new_string);
        string* pointer_to_input = new string(new_string);
        link * current_link = new link(head_pointer, pointer_to_input);
        head_pointer = current_link;
    }

    link::printAll(head_pointer);
    return 42;
}

In short to avoid errors you shouldn't store pointers to strings in your link, you should just store strings themselves. Your links perhaps shouldn't assume ownership of other links:

struct link
{
    link *next;
    string value;

    link(link *next, const std::string& value) : next(next), value(value) {}

    link * getNext();
    const std::string& getString() const;
    static void printAll(link *link_to_print);
};

link * link::getNext() {
    return next;
}
const string& link::getString() const {
    return value;
}
void link::printAll(link *link_to_print)
{
    if (!link_to_print)
        return;
    cout << link_to_print->getString() << endl;
    printAll(link_to_print->next);
}

and your main:

int main()
{
    link * head_pointer = new link(NULL, "");
    for (int i = 0; i < 10; i++) {
        string new_string;
        getline(cin, new_string);
        link * current_link = new link(head_pointer, new_string);
        head_pointer = current_link;
    }

    link::printAll(head_pointer);

    // TODO: now you need to walk head_pointer and delete all links manually.
    return 42;
}

Once you learn how memory management works in general you should most likely redesign your link using some kind of smart pointer helper class, such as unique_ptr or shared_ptr. And off course, once you master linked list you should start using std::list.

edited Apr 29, 2017 at 4:28

answered Apr 29, 2017 at 3:54

Pavel P

17.3k11 gold badges91 silver badges143 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

2____ Over a year ago

That did the trick... Wow I had no idea that that could cause a Segmentation Fault, I thought the error would be something about dereferencing...thanks

Ben Voigt Over a year ago

"you shouldn't delete in destructor if you didn't allocate that in constructor" is a terrible rule. If an object takes ownership of constructor arguments, like std::unique_ptr does, then it is perfectly normal for its destructor to free them.

Pavel P Over a year ago

@BenVoigt if ctor takes onwnership then copy constructor and assignment operators have to be defined (or deleted). In any case, without link.h it's difficult to tell, but overall looks like this code should be revisited.

Pavel P Over a year ago

@BenVoigt where is the infinite recursion?

Ben Voigt Over a year ago

@Pavel: Yes, that's the real rule, commonly called the "Rule of Three" (and became "Rule of Five" since C++11, since there are also move constructor and move assignment to worry about)

|

Ben Voigt · Accepted Answer · 2017-04-29 04:09:05Z

1

link::printAll takes its argument by value, which has two important effects:

The argument inside the function is a second link object created by making copies of the same value and next pointer.
The copy has automatic storage duration and is destroyed at the end of the function call.

Therefore, you have double frees going on. In particular, both the copy made in the recursive call and the sub-link of the original link share the same value pointer, and both try to delete it. The second deletion causes undefined behavior.

The solution is to respect the rule-of-three and not allow shallow copies of raw pointers. There are two possible approaches for managing objects owned by pointer:

Write a copy constructor to go with your destructor, so the two deletes mentioned above act on two different copies of the value.

OR

Use a smart pointer, such as std::shared_ptr, so you don't have to write a destructor by hand at all.

Note that you need a pointer to implement the connection between objects in the linked list, but you do not need a pointer to store the data. Having a data member of type std::string, instead of std::string *, would be just fine and do the right thing when copied (It makes sense to think of std::string as a smart pointer to an array of characters, that just happens to also have some extra string-manipulation functions tacked on).

answered Apr 29, 2017 at 4:09

Ben Voigt

286k45 gold badges444 silver badges764 bronze badges

1 Comment

Pavel P Over a year ago

I agree in general, but IMO shared_ptr is a heavy weight thing that I'd rather try to avoid it; Thought, it would make things easy for a simple list attempt.

Collectives™ on Stack Overflow

C++ Segmentation Fault From Null Pointer Solution?

2 Answers 2

6 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related