Trying to pass 2D array of chars, but getting garbage

Question

I am trying to parse a std::string, split it, and then store it in a 2D char array. The first row of this array, will contain the total number of rows. I dynamically allocate the array inside the getC_strings() function, and when I print it, I get the expected results. However, when I am printing again from the main(), I get garbage, for rows 0, 2. What am I doing wrong?

#include <iostream>
#include <string>
#include <vector>
#include <boost/algorithm/string/classification.hpp> // Include boost::for is_any_of
#include <boost/algorithm/string/split.hpp>          // Include for boost::split

using namespace std;

/**
*
* @param input a string separated by spaces
* @param numArgs_ an int
* @param cargs a const char ** something.  Pass it by its address aka &something.
*/
static inline void getC_strings(const std::string & input, int & numArgs_, const char *** cargs) {

    std::vector<std::string> args;
    boost::split(args, input, boost::is_any_of(" "), boost::token_compress_on);
    numArgs_ = int(args.size());
    *cargs = new const char* [numArgs_ + 1];


    // store the number of rows at the first row
    (*cargs)[0] = new char[to_string(numArgs_).size()];
    (*cargs)[0] = to_string(numArgs_).c_str();

    // write the characters from the vector per row
    int ind = 0;
    for(auto const &v:args) {
        ind++;
        (*cargs)[ind] = new char [int(v.size())];
        if((*cargs)[ind] == NULL) std::cout << "OUT OF MEMORY! " << std::endl;
        (*cargs)[ind] = const_cast<char*>(v.c_str());
    }


    for(int i = 0; i < numArgs_; ++i) {
        std::cout << i << " " << (*cargs)[i] << std::endl;
    }

}


int main () {

    string arg = "test ./MyDirectoryName/OPQ_Arksoatn.txt 1 SOMETHING 1 2 3 4 5 6 7";
    int numCargs = 0;
    const char ** cargs;
    getC_strings(arg, numCargs, &cargs);

    cout << "  ==============================================" << endl;
    for(int i = 0; i < numCargs; ++i) {
        std::cout << i << " " << cargs[i] << std::endl;
    }

    return 0;
}

OUTPUT:

    0 11
    1 test
    2 ./MyDirectoryName/OPQ_Arksoatn.txt
    3 1
    4 SOMETHING
    5 1
    6 2
    7 3
    8 4
    9 5
    10 6
    ==============================================
    0 ��`
    1 test
    2 `��
    3 1
    4 SOMETHING
    5 1
    6 2
    7 3
    8 4
    9 5
    10 6

The default `new` operators never return null - they throw an exception if they fail. Don't check for null result. — aschepler, Mar 16 '17 at 23:28
Why are you incrementing `ind` at the begging of the loop? Do you want to skip the first place of the array? — Angelos, Mar 16 '17 at 23:34
Yes, at the first row I just want to store the number of rows. — nikferrari, Mar 16 '17 at 23:37
Your function has memory leaks. Why are you using `new` at all? Why not use `std::vector`? — PaulMcKenzie, Mar 17 '17 at 00:38
I am interfacing with an old code that expect const char ** as input arguments — nikferrari, Mar 17 '17 at 01:41
@nikferrari -- The solution can still be crafted without the memory leaks in your code. A `std::vector`, where the internal `const char *` is actually a pointer to a buffer that automatically cleans itself up when it goes out of scope would be one way to provide a `const char**`. — PaulMcKenzie, Mar 17 '17 at 10:57
@nikferrari I posted a solution that does not leak memory. It is merely a class wrapper that takes advantage of `std::list` and `std::vector`. — PaulMcKenzie, Mar 17 '17 at 11:35

score 2 · Answer 1 · edited May 23 '17 at 12:32

You can try this method that does not leak memory. It is crafted from the solution found here.

#include <iostream>
#include <string>
#include <vector>
#include <list>
#include <boost/algorithm/string/classification.hpp> // Include boost::for is_any_of
#include <boost/algorithm/string/split.hpp>          // Include for boost::split

using namespace std;

class CharStarWrapper
{
    private:
        typedef std::vector<char> CharArray;
        typedef std::list<CharArray> StringList;
        typedef std::vector<const char *> ArgList;
        const char** m_Args;
        StringList m_sList;
        ArgList m_cStrings;

    public:
        CharStarWrapper(const std::string & input) : m_Args(nullptr)
        {
            std::vector<std::string> args;
            boost::split(args, input, boost::is_any_of(" "), boost::token_compress_on);
            for (auto const &v : args)
            {
                // create an array of char and place on list
                m_sList.push_back(CharArray(v.begin(), v.end()));

                // null terminate this entry
                m_sList.back().push_back(0);

                // add the pointer to this entry to the vector of const char *.
                m_cStrings.push_back(&m_sList.back()[0]);
            }
            m_Args = m_cStrings.data();
        }

        const char** getArgs() { return m_Args;  }
        int getArgCount() const { return static_cast<int>(m_cStrings.size()); }
};

void fake_main(int argc, const char **argv)
{
   std::cout << "The number of arguments is " << argc << "\n";
   for (int i = 0; i < argc; ++i) 
        std::cout << argv[i] << "\n";
}

int main() {
    string arg = "test ./MyDirectoryName/OPQ_Arksoatn.txt 1 SOMETHING 1 2 3 4 5 6 7";
    CharStarWrapper wrapper(arg);
    fake_main(wrapper.getArgCount(), wrapper.getArgs());        
}

Live Example

Basically, we wrap the const char** in a class. This class maintains the dynamic array of strings, and merely provides public member functions to return the const char** as well as the number of arguments. A call to a "fake" main() function demonstrates the usage.

There are no calls to new[], delete[], strdup, etc. that would require calls to the deallocation routines to avoid memory leaks. When the wrapper goes out of scope, all of the memory is cleaned up automatically.

Note that this solution relies on the wrapper object not going out of scope for the lifetime you will be using the const char ** value. The reason is that CharStarWrapper maintains the infrastructure, and destroying the infrastructure while using it will be incorrect.

score 1 · Accepted Answer · answered Mar 16 '17 at 23:24

1

In a couple of places:

// store the number of rows at the first row
(*cargs)[0] = new char[to_string(numArgs_).size()];
(*cargs)[0] = to_string(numArgs_).c_str();

and

   // Similar code that allocates then ignores some space
   (*cargs)[ind] = const_cast<char*>(v.c_str());

You are carrying off a pointer to an internal part of a std::string. You need to duplicate this into your array to walk off with it in your char array structure. (see strdup).

Replace with:

(*cargs)[0] = strdup(to_string(numArgs_).c_str());

and

   (*cargs)[ind] = strdup(v.c_str());

answered Mar 16 '17 at 23:24

Art Yerkes

1,274
9
9

BTW, is there any standard (and portable) function that can achieve it? – nikferrari Mar 16 '17 at 23:50
strdup is in posix, or are you asking about putting the whole structure together? – Art Yerkes Mar 17 '17 at 01:07
1

@nikferrari The "standard" way is to use C++ and wrap this in a class that maintains the infrastructure. The other answer that I've posted demonstrates this. – PaulMcKenzie Mar 17 '17 at 17:18
Definitely endorse a class wrapper. – Art Yerkes Mar 17 '17 at 18:26

Trying to pass 2D array of chars, but getting garbage

2 Answers2