Why do I get a segmentation fault while iterating through this string?

Question

I'm trying to implement a basic shift cipher in C++. I can't move forward until I figure out what's causing the segmentation fault. I stepped through the code using gdb and the problem seems to stem from the iterator.

 1 #include <iostream>
 2 #include <string>
 3 
 4 std::string encrypt (std::string plain, int key);
 5 
 6 int main()
 7 {
 8         std::string plaintext;
 9         std::getline(std::cin,plaintext,'\n');
 10        encrypt(plaintext,3);   
 11 }
 12 
 13 std::string encrypt(std::string plain, int key)
 14 {
 15         std::string::iterator ic;
 16         for (ic= plain.begin(); ic != plain.end();++ic)
 17         {
 18                 std::cout <<*ic + key << std::endl;
 19         }
 20 }

Error:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7b73ef1 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() () from /usr/lib/libstdc++.so.6

Answer 1

You have declared encrypt as returning an std::string, but you don't return anything from the function. You need to return a value or you need to change the return type to be void to indicate that the function doesn't return anything.

As for why it's crashing as written, I can only speculate. It is likely that the compiler has generated a call to the std::string destructor in main to clean up the std::string object that encrypt returns. Since encrypt doesn't actually return anything, the destructor ends up getting called for an object that doesn't exist. The memory that should contain the object likely just contains garbage data and the destructor doesn't like that.

Answer 2

Writing a loop, but changing it from using an index to using an iterator is usually a mistake. If you're going to use an explicit loop, it usually makes more sense to just continue to use an index, at least for something like string that allows random access. The primary point of iterators is to enable generic algorithms that aren't coupled to containers. To make good use of an iterator, use it with an algorithm:

int main() { 
    std::string plaintext;
    std::getline(std::cin, plaintext);

    std::transform(plaintext.begin(), plaintext.end(), 
                   std::ostream_iterator<char>(std::cout), 
                   std::bind2nd(std::plus<int>(), 3));
    return 0;
}

Edit: Since Hans Passant brought it up, a version using a lambda expression would look like this:

std::transform(line.begin(), line.end(), 
               std::ostream_iterator<char>(std::cout), 
               [](char ch) { return ch+'\03'; });

Only relatively recent compilers support this though -- gcc >= 4.6, Visual Studio >= 2010.

Answer 3

[C++2003 Standard section 6.6.3-2] Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function.

This is a very easy mistake to make if your compiler doesn't warn you. You can save yourself a lot of debugging by enabling as many compiler warnings as possible. In the case of gcc/g++, I suggest compiling with "-Wall -Werror" for any new code that you are writing. With those options, compilation of this program fails with the following message:

cc1plus: warnings being treated as errors
In function 'std::string encrypt(std::string, int)':
Line 20: warning: control reaches end of non-void function

Answer 4

The problem is that your encrypt function lacks a return statement.

Add

return "blah blah";

In addition to fixing that, consider passing strings by reference to const, like

std::string const& plain

Cheers & hth.,