Segmentation fault while iterating a std::set in C++

Question

This portion of my code (for this project) gives me a segmentation fault. The source code is available here.

void PackageManager::install_package(string pname)
{
  if(repository->exists_package(pname)) {
    Package *pkg;
    ConcretePackage *cpkg;
    MetaPackage *mpkg;
    if(repository->is_virtual(pname)) {
      //code for dealing with meta packages
      mpkg = new MetaPackage(pname);
      pkg = mpkg;
      system->operator+(pname);
    } else {
      //code for dealing with concrete packages
      cpkg = new ConcretePackage(pname);
      pkg = cpkg;
      system->operator+(pname);
      if( cpkg->getDependencies().size() > 0) {
        for(set<string>::iterator sit = pkg->getDependencies().begin();
            sit!=pkg->getDependencies().end(); ++sit) {
          cout<<*sit<<endl;
          system->operator+(*sit);
        }
      }
    }
  } else {
    cout<<"Invalid Package Name"<<endl;
  }
}

Here is the error when I run gdb and also the backtrace.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7b6db03 in std::basic_ostream<char, std::char_traits<char> >& std::operator<< <char, std::char_traits<char>, std::allocator<char> >(std::basic_ostream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /usr/lib/libstdc++.so.6
(gdb) backtrace
#0  0x00007ffff7b6db03 in std::basic_ostream<char, std::char_traits<char> >& std::operator<< <char, std::char_traits<char>, std::allocator<char> >(std::basic_ostream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /usr/lib/libstdc++.so.6
#1  0x00000000004052e8 in PackageManager::install_package (this=0x7fffffffe280, pname=...) at packagemanager.cpp:39
#2  0x000000000040575a in main () at packagemanager.cpp:79

I'm trying to iterate through a set and perform some operation. I can pode more code if needed. I would also like it if someone could guide me to a place where I could learn to understand these segfaults. I don't know much about them and I tend to panic when I encounter these.

This is the operator+ for the System class.

void System::operator+(string pname)
{
  installed_packages.insert(pname);
  log.push_back("Added " + pname);
}

I know the design isn't the best but I'm trying to implement the items of a checklist for this project, which covers various areas of Object Oriented Programming. The checklist is also available on github.

I have tried to run the code through the debugger, printng out *sit. It works for sometime and then crashes. I don't know too much about gdb.

Answer 1

StackOverflow has a few "What is a segmentation fault?" style Q&A:

What is a segmentation fault?

Ideally you are working in an environment with a debugger and the ability to step through your code line-by-line, or place breakpoints. This can help you isolate the circumstances surrounding your crash. You already had a line number in the stack trace--which we are assuming points a smoking gun at:

cout<<*sit<<endl;

But stepping through with a debugger could answer questions like whether this happens the first time through the loop...and if not then on which element.

---

UPDATE: Looking at the pieces of this code you have on GitHub (which doesn't include the above code), I see that ConcretePackage::getDependencies() is returning a set by value and not by reference. This means that each time you call the member, you get a new copy of the set. Iterators from different containers should not be compared with each other, even if they are the same type:

comparing iterators from different containers

To address this, you could change:

for(set<string>::iterator sit = pkg->getDependencies().begin();
        sit!=pkg->getDependencies().end(); ++sit) { ... }

...into:

set<string> deps = pkg->getDependencies();
for(set<string>::iterator sit = deps.begin(); sit!=deps.end(); ++sit) { ... }

...or you could change your definition of getDependencies to return a reference:

set<string>& ConcretePackage::getDependencies() {
    return dependencies;
}

Studying the reasons to do it one way vs. another is left as an exercise to the student. :P

---

A few more notes:

• You don't need a special-case test against zero size for collection classes you're going to use iterators with. If a set contains no elements, then the .begin() of that set will return an iterator that is equal to the .end(). The loop you have above handles this case fine and would exit immediately.

• Explicitly calling operator+ in your code without doing anything with the return value suggests you probably have some kind of side-effect. Few people expect expressions like a = b + c to change b or c...and a single line of code like b->operator+(c); suggests you're doing something of the sort. While technically possible, I'd avoid it. See point #2 here: Operator overloading

• When you post code samples try and keep them readable and not needing a lot of scroll bars to display. Break lines up if you notice in the preview that it's putting a long horizontal scroll bar. Instead of using separate lines for each brace, put them on the same lines as the condition. (Regardless of what convention you use in your codebase, the briefer the better when asking for technical help online.)

(Also provide context. If you don't say it's homework and a design of your own, then people like me will go Googling to try and figure what sort of package manager you are using. Luckily I found your programmers.stackexchange.com post...)