What's wrong with my implementation of std::strcopy? (segmentation fault)

Question

I'm encountering a run time error attempting to use std::strcopy with elements in a vector of std::string.

There is no problem with the vector. I have higher level functions that work without a hitch. I'm running into an issue with my low level function char ** argv().

Here is a chunk of a class I'm writing. I think I've posted enough of it for the question. I'm trying to focus attention to the problem.

At runtime, the line indicated in the code below blows up.

class ArgParser{
    public:

        ... MORE CODE ...

        int & argc()
        {
            argc_ = exePath_.empty() ? 0 : 1 + args_.size();
            return argc_;
        }

        char ** argv()
        {
            const int argCount = argc();
            if( argCount==0 ) return argv_;
            if( argv_ )
            {
                for( int i=0; i < argCount; i++ )
                    delete argv_[i];
                delete argv_;
            }
            argv_ = new char*[argCount];
            *(argv_ + 0)=new char[ exePath().size() ];
            strcpy( *(argv_ + 0), exePath_.c_str() );
            int i=1;
            for( auto &arg : args_ )
            {
                *(argv_ + i++)=new char[ arg.size() ];
                strcpy( *(argv_ + i++), arg.c_str() ); // SEG FAULT!
            }
            return argv_;
        }
    private:
        int argc_;
        char **argv_;
        std::vector <std::string> args_;
        std::string exePath_;
};

Answer 1

The following lines suffer from couple of problems.

*(argv_ + i++)=new char[ arg.size() ];
strcpy( *(argv_ + i++), arg.c_str() ); // SEG FAULT!

1. i is incremented twice. Let's say i is 0 before those two lines. The first line allocates memory for argv_[0]. i is incremented and its value becomes 1. In the second line you try to copy to argv_[1] and i is incremented again. That is a problem since you have not allocated memory for argv_[1] yet.

   That leads to further problems. In the next iteration of the for loop, you access argv_[2] and argv_[3], which compounds the problem further since they might be invalid indices for argv_.

   You can fix that by incrementing i after the lines have been executed. As a coding practice, using i++ and ++i in such places is best avoided.

2. The first line does not allocate enough space. It needs one more character to hold the terminating null character.

Change those lines to:

argv_[i] = new char[arg.size() + 1];
strcpy(argv_[i], arg.c_str());
++i;

Answer 2

You don't assign argv_[1] to argv_[argCount-1].

You increment i twice, so you still dereference an invalid pointer

You don't allocate enough space for the terminating '\0'.

Stop doing things like you are writing C. There is a reason std::string and std::vector have data members. Use them.

"Some other library ... erases the arguments" If it calls delete on pointers it didn't new, you should stop using it, because that's undefined behaviour waiting to happen. If it doesn't, and argv() is called once per instance, you can just let it play around in the data that args_ allocated

Answer 3

Doing things the right way™

Another answers already explain mistakes in your implementation.

In this answer I just want to show you a simpler way of implementing the same thing, without the obnoxious (i.e. any) amount of manual allocation:

int argc_;
std::vector <std::string> args_;
std::string exePath_;
// New fields:
std::vector<std::string> argv_data_;
std::vector<char *> argv_;

char **argv()
{
    argv_data_.clear();
    argv_data_.push_back(exePath_);
    argv_data_.insert(argv_data_.end(), args_.begin(), args_.end());
    argv_.clear();
    for (auto &it : argv_data_)
        argv_.push_back(it.c_str());
    argv_.push_back(0); // The standard `argv` is null-terminated, we should do it to.
    return argv_.data();
}

That's all. No new, no risk to leak anything.

This code still allows your C api to modify argv[i] and argv[i][j] safely, exactly as if it was a plain argv received by main().

Answer 4

Here's what I went with, which worked. It's very much like what HolyBlackCat posted.

class ArgParser{
    public:
    ...

        int & argc()
        {
            argc_ = exePath_.empty() ? 0 : 1 + args_.size();
            return argc_;
        }

        char ** argv()
        {
            if( argc()==0 ) return 0;
            argv_.clear();
            argvData_.clear();
            argvData_.push_back( exePath_ );
            for( auto arg : args_ )
                argvData_.push_back( arg );
            for( auto & arg : argvData_ )
                argv_.push_back( &arg.front() );
            return argv_.data();
        }

    private:
        std::string exePath_;
        std::vector <std::string> args_;

        int argc_;
        std::vector <std::string> argvData_;
        std::vector <char*> argv_;
};