Re: Does this look scary to you?
Okay, I've rewritten the function to hopefully be cleaner.
Please give me your thoughts.
STDMETHODIMP CForm::GetStringValue(LONG lFieldID, /* [out,retval] */ BSTR*
pBstr)
{
if (!pBstr)
return E_POINTER;
_ASSERTE(!(*pBstr) && _T("This should be NULL! It's either not
initialized or is already pointing to allocated memory!"));
CString csDesc = GetRuntimeText(lFieldID);
if (csDesc.IsEmpty())
return E_FAIL; // Is this really a failure?
try
{
*pBstr = csDesc.AllocSysString();
}
catch(CMemoryException* pme)
{
// NEVER throw an exception over a COM boundary.
*pBstr = NULL;
pme->Delete();
} // catch (CMemoryException*)
catch(...)
{
// NEVER throw an exception over a COM boundary.
*pBstr = NULL;
} // catch everything else
if (*pBstr == NULL)
return E_OUTOFMEMORY;
return S_OK;
}
"SvenC" <SvenC@community.nospam> wrote in message
news:66455A59-8729-4033-9D40-4863E210D6E5@microsoft.com...
Leo V wrote:
I'm reviewing some code and I'm concerned about how the code is
dealing with returning a BSTR value. Consider the following function
(shorter version of the real function).
STDMETHODIMP CForm::GetStringValue(LONG lFieldID, BSTR* pBstr)
{
if (!pBstr)
return E_POINTER;
CString csDesc = GetRuntimeText(lFieldID);
*pBstr = (*pBstr ? csDesc.SetSysString(pBstr) :
csDesc.AllocSysString());
if (csDesc.IsEmpty())
return E_FAIL;
else
return S_OK;
}
My first concern is the line:
*pBstr = (*pBstr ? csDesc.SetSysString(pBstr) :
csDesc.AllocSysString());
This seems to be assuming its an input/output parameter. Wouldn't
this code likely crash if someone just declared a BSTR variable
(without assigning it NULL) and called this function? It might
interpret random data as a previously allocated BSTR that must be
freed via SetSysString.
Do you have a typelib where the method is defined? Is it attributes as
[out, retval], [out] or [in, out]? If not you should make that explicit.
If I remember correctly [out] is not automation compatible, so [in, out]
or [out, retval] might be intended.
Further, this code
if (csDesc.IsEmpty())
return E_FAIL;
else
return S_OK;
will return E_FAIL even though memory was allocated. Most code I
know of would probably not realize they need to free the BSTR
returned if the HRESULT on the function wasn't S_OK. This to me
seems like it could result in a memory leak if someone wasn't careful.
I would change that, also. Something like return *pBstr == NULL ?
E_OUTOFMEMORY : S_OK;
CStrings BSTR methods will throw a CMemoryException when insufficient
memory is available. So the assignment to *pBstr should be put in a
try/catch block, setting *pBstr = NULL in the catch block.
--
SvenC