lwiu lwiu - 1 month ago 11
C++ Question

VC++ class objects with a VARIANT member have strange behavior

I have a class defined below:

class CVariable
{
public:
CVariable(CString strData, int nNum);
CVariable(BSTR bsData);
~CVariable();
public:
VARIANT GetVariant(){return m_bsVa;};
private:
VARIANT m_bsVa;
VARIANT m_nVa;
};


And implements are:

CVariable::CVariable(CString strData, int nNum)
{
VariantInit(&m_bsVa);
BSTR bsData = ::SysAllocString(strData);
m_bsVa.vt = VT_BSTR;
m_bsVa.bstrVal = bsData;
::SysFreeString(bsData);

VariantInit(&m_nVa);
m_nVa.vt = VT_I2;
m_nVa.lVal = nNum;
}

CVariable::CVariable(BSTR bsData) {
m_bsVa.vt = VT_BSTR;
m_bsVa.bstrVal = bsData;
}

CVariable::~CVariable()
{
VariantClear(&m_bsVa);
VariantClear(&m_nVa);
}


When I try to construct two instances using constructor
CVariable(CString,int)
,
class member m_bsVas always have the same value,while m_nVas are different.The result is below:
enter image description here

As you see, v1 and v2 have the same m_bsVa but different m_nVa, while using constructor
CVariable(BSTR)
leads to the right result. I've no idea why this can happen?
Any help will be appreciated.

Answer

I see several problems with your code.

  1. the CVariable(CString, int) constructor allocates a BSTR for m_bsVa, but then frees the BSTR immediately, leaving m_bsVa pointing at invalid memory, and allowing the next CVariable instance to potentially reuse the same memory address for its allocated BSTR. You need to leave the BSTR allocated until you are done using m_bsVa (or at least until you want to assign a new value to it). VariantClear() will free the BSTR for you.

  2. the CVariable(BSTR) constructor is not initializing m_nVa at all, which will cause problems for subsequent operations on it, including VariantClear(). Also, the constructor is taking ownership of the caller's BSTR. That may or may not be OK, depending on how you use this constructor. If the caller is not expecting you to take ownership, then you need to make a copy of the BSTR using SysAllocString/Len().

  3. VARIANT is not trivially copyable. You need to use the VariantCopy() function to copy data from one VARIANT to another. That means your CVariable class needs to implement a copy constructor and copy assignment operator. Which you need to do anyway so your class conforms to the Rule of Three.

  4. GetVariant() is returning m_bsVa as-is, so the compiler will simply copy the values of m_bsVa's fields as-is into the caller's receiving VARIANT. Since BSTR is a pointer, the caller will have direct access to the original BSTR inside of your class. That may or may not be OK, depending on how you use GetVariant(). In the current implementation, any access to the returned BSTR should be treated as read-only - the caller must not call SysFreeString() on it, and must expect any change to the CVariable object may invalidate the BSTR. If that does not suit your needs, then GetVariant() should return a new VARIANT that has copied the data via VariantCopy(), and then the caller can call VariantClear() when done using the returned VARIANT.

With that said, try something more like this:

class CVariable  
{
public:
    CVariable(const CString &strData, int nNum);
    CVariable(BSTR bsData);
    CVariable(const CVariable &src);
    ~CVariable();

    VARIANT GetVariant() const;

    CVariable& operator=(const CVariable &src);
    CVariable& operator=(BSTR src);

private:
    VARIANT m_bsVa;
    VARIANT m_nVa;
};

CVariable::CVariable(const CString &strData, int nNum)
{
    ::VariantInit(&m_bsVa);
    m_bsVa.vt = VT_BSTR;
    m_bsVa.bstrVal = ::SysAllocString(strData);

    ::VariantInit(&m_nVa);
    m_nVa.vt = VT_I2;
    m_nVa.lVal = nNum;
}

CVariable::CVariable(BSTR bsData)
{
    ::VariantInit(&m_bsVa);
    m_bsVa.vt = VT_BSTR;
    m_bsVa.bstrVal = bsData;
    /* or this, if needed:
    m_bsVa.bstrVal = ::SysAllocStringLen(bsData, ::SysStringLen(bsData));
    */

    ::VariantInit(&m_nVa);
}

CVariable::~CVariable()
{
    ::VariantClear(&m_bsVa);
    ::VariantClear(&m_nVa);
}

VARIANT CVariable::GetVariant() const
{
    return m_bsVa;
    /* or this, if needed:
    VARIANT result;
    ::VariantInit(&result);
    ::VariantCopy(&result, &m_bsVa);
    return result;
    */
}

CVariable& CVariable::operator=(const CVariable &src)
{
    if (&src != this)
    {
        ::VariantClear(&m_bsVa);
        ::VariantCopy(&m_bsVa, &src.m_bsVa);

        ::VariantClear(&m_nVa);
        ::VariantCopy(&m_nVa, &src.m_nVa);
    }

    return *this;
}

CVariable& CVariable::operator=(BSTR src)
{
    ::VariantClear(&m_bsVa);
    m_bsVa.vt = VT_BSTR;
    m_bsVa.bstrVal = src;
    /* or this, if needed:
    m_bsVa.bstrVal = ::SysAllocStringLen(src, ::SysStringLen(src));
    */

    ::VariantClear(&m_nVa);

    return *this;
}

If you use the variant_t class instead of VARIANT directly, you can greatly simplify the code while still addressing all of the points mentioned above:

class CVariable  
{
public:
    CVariable(const CString &strData, int nNum);
    CVariable(BSTR bsData);

    variant_t GetVariant() const;

private:
    variant_t m_bsVa;
    variant_t m_nVa;
};

CVariable::CVariable(const CString &strData, int nNum)
    : m_bsVa(strData), m_nVa(nNum)
{
}

CVariable::CVariable(BSTR bsData)
    : m_bsVa(bsData)
{
}

variant_t CVariable::GetVariant() const
{
    return m_bsVa;
}
Comments