invalid read size of 1 valgrind

Question

I've been scratching my hair out but can't seem to find what is wrong with the following code. Here is the small snippet of valgrind output that it generates

Invalid read of size 1 at 0x4c22d82: strlen (mc_replace_strmem.c:242) by 0x5E65CA: Application::readConfigurationFile() (char_traits.h:262) by 0x5694BD: main Address 0xafc9660 is 24 bytes inside a block of size 39 free'd at 0x4C20E0D: operator delete(void*) (vg_replace_malloc.c:342) by 0x635618: Configurator::getParameterValue(char const*, char**) by 0x5E65B2: Application:readConfigurationFile() (Application.cpp:77) by 0x5694BD: main

bool Configurator::getParameterValue(const char *p_pParameterName, char** p_pParameterValue)
{
    bool blReturnValue = false;

    QDomElement element;
    QDomNode node;
    QDomNodeList list;

    list = doc.elementsByTagName(p_pParameterName);
    if (!list.isEmpty())  
    {
        node = list.item(0);
        element = node.toElement();
        QString qs = element.text().toUtf8();
        *p_pParameterValue = (char *)(qs.toStdString().c_str());
        blReturnValue = true;
    }
    else
    {
        char sMessage[200];
        sprintf(sMessage, "<Configurator::getParameterValue> Error! Parameter %s could not be found\n", p_pParameterName);
        m_outputFunction(sMessage);
    }

    return blReturnValue;
}

bool Configurator::parseFile()
{
    bool blReturnValue = false;

    QString errorStr;
    int errorLine;
    int errorColumn;

    if (!doc.setContent((QIODevice*)(&file), true, &errorStr, &errorLine, &errorColumn))
    {
        char aTemp[512];
        sprintf(aTemp, "<Configurator::parseFile> error! can not read the file row: %d, column: %d:\n",errorLine, errorColumn);
        m_outputFunction(aTemp);
    }
    else
    {
        closeFile();
        blReturnValue = true;
    }

    return blReturnValue;
}

bool Application::readConfigurationFile()
{
    bool blReturnValue = false;

    m_outputFunction("<Application::readConfigurationFile> Reading configuration..\n");

    if(m_configurator.parseFile())
    {
        blReturnValue = true;

        m_configurator.writeParameters();

        char *pTemp = 0;


        if(!m_configurator.getParameterValue("center_no", m_bCenterNo)) 
            m_bCenterNo = 1;
        if(m_configurator.getParameterValue("highway_header", &pTemp))
            m_strHighwayHeader.assign(pTemp);
        else
            m_strHighwayHeader.assign("... HIGHWAY"); // Default value
    }
    return blReturnValue;
}

Can somebody please tell me why I see invalid reads, I don't even use malloc/calloc in this code snippet.

Answer 1

*p_pParameterValue = (char *)(qs.toStdString().c_str());

Why you did so? QString is local variable and toStdString return new std::string

std::string QString::toStdString () const

So, returned std::string will be deleted. c_str() returns pointer to const char*. Quote from n3337 draft:

const charT* c_str() const noexcept;
const charT* data() const noexcept; 

1 Returns: A pointer p such that p + i == &operator[](i) for each i in [0,size()]. 2 Complexity: constant time. 3 Requires: The program shall not alter any of the values stored in the character array.

if(m_configurator.getParameterValue("highway_header", &pTemp))
                m_strHighwayHeader.assign(pTemp);

Wrong. Since value in pTemp was deleted, when temporary object qs.toStdString() was deleted.

Answer 2

The temporary string object returned from qs.toStdString() allocates memory for the string, which is freed when the temporary is destroyed (after evaluation of the full expression). If you are compiling with optimization, it is likely that the std::string d'tor is inlined into your function, so it doesn't show up in your call stack.

When you want to continue using the string data after the function is done, you need to make it persist. The (in my opinion) sanest way is to return a std::string object rather than a char *, so the last parameter could be a std::string ** (which is filled by new std::string(qs.toStdString())), or a std::string & which is assigned to.

If you have the Boost libraries, you can also use boost::optional<std::string> as the return type, which provides a "string with a valid flag" data type.

Answer 3

You are in effect returning a pointer to a local variable. In getParameterValue the variable qs is local inside a block, and you assign that strings pointer to p_pParameterValue. When getParameterValue the stack space formerly occupied by qs is now reclaimed and the pointer pTemp now points to unused memory. This is undefined behavior, and can cause lots of bad stuff to happen.