-1

Using the following code mentioned in https://stackoverflow.com/a/236803/6361644, I wrote the following code to parse a string into a vector, where each element is separated by white space.

std::string line = "ls -l -a";
std::string cmd;
std::vector<char*> argv;
std::stringstream ss;
ss.str(line); 
std::string tmp;
getline(ss, cmd, ' ');
argv.push_back( const_cast<char*>(cmd.c_str() ) );
while(getline(ss, tmp, ' '))
    argv.push_back( const_cast<char*>(tmp.c_str() ) );
argv.push_back(NULL);

Printing argv after this code gives

{gdb) print argv                                                                         
$22 = std::vector of length 3, capacity 4 = {0x26014 "ls", 0x2602c "-a", 0x2602c "-a", 0x0}

I'm not sure why the second element is being overwritten. Any tips would be appreciated.

Community
  • 1
  • 1
frost
  • 56
  • 5

2 Answers2

2

You're storing dangling pointers (in an ill-formed way no less! the proper way to store pointers to c-style strings is const char*, not char*).

In this (const-corrected) loop:

std::vector<const char*> argv;
// ...
while(getline(ss, tmp, ' '))
    argv.push_back(tmp.c_str());

every subsequent iteration will clear tmp, invalidating the previous pointer that you had stored. Every tmp.c_str() you pushed back is immediately freed by getline(). So all subsequent accesses are undefined.

You have to take ownership of all the strings, you can do so by instead storing the full string:

std::vector<std::string> argv;
// ...
while(getline(ss, tmp, ' '))
    argv.push_back(std::move(tmp));

And now argv actually owns all of its own resources.

Barry
  • 286,269
  • 29
  • 621
  • 977
-1

The pointer returned by c_str() points to a std::string's internal data.

This pointer is only valid until the string is destroyed, or modified. Once the std::string is destroyed, or modified, the pointer is no longer valid.

while(getline(ss, tmp, ' '))
    argv.push_back( const_cast<char*>(tmp.c_str() ) );

Setting aside the issue of casting the const-ness away, which is already a red flag: each time the while loop iterates the contents of tmp get replaced by the next line in the ss file.

This automatically invalidates the c_str() that was obtained on the previous iteration of the while loop.

The correct solution here is to parse all the individual words into a std::vector<std::string>, first.

Then, once this vector is initialized, iterate over the vector and obtain each individual string's c_str(), to construct the vector of raw character pointers.

Even better: use a std::vector<char> instead of a std::string, add an explicit '\0' character at the end of each vector, and the ugly const_cast will not be necessary.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • Why would you suggest `vector` to store null-terminated strings? That's exactly what `string` is for. -1. – Barry Oct 01 '16 at 22:52
  • Because he needs a `char *`, for an obvious `exec()`. – Sam Varshavchik Oct 01 '16 at 22:53
  • 1
    Using the wrong container and manually mucking around with null terminators is much uglier than a `const_cast` – M.M Oct 01 '16 at 23:53
  • That's a matter of opinion. My opinion is that using a `std::vector` to set up a vector of parameters to `exec()`, after which the process will not exist, is cleaner than an ugly `const_cast`. – Sam Varshavchik Oct 02 '16 at 01:14