Spot the Defect, Part Two

Spot the Defect, Part Two

  • Comments 22

Some fun for a Monday -- for some definition of fun, I suppose.  Here's a recent posting to Microsoft's internal Spot The Defect mailing list.

HRESULT CInvokeHelper::InvokeHelper(IDispatch *pDisp, long dispid, SAFEARRAY **param1)
{
    HRESULT hr;
    DISPPARAMS params;
    EXCEPINFO hrInfo;
    VARIANTARG args[1];
    params.cArgs=1;
    params.rgvarg=args; 
    params.cNamedArgs=0;
    params.rgvarg[0].vt=VT_SAFEARRAY | VT_I4;
    params.rgvarg[0].parray = *param1;
    hr = pDisp->Invoke(dispid,IID_NULL, LOCALE_USER_DEFAULT, 
       DISPATCH_METHOD, &params, NULL, &hrInfo, NULL);
    return returnVal(hr, hrInfo.scode); 
}

HRESULT CInvokeHelper::returnVal(HRESULT invokeHr, HRESULT scodeHR)
{
    m_invokeResult = invokeHr;        
    if(FAILED(invokeHr))                  
    {
        if(DISP_E_EXCEPTION == invokeHr) 
            return scodeHR;    
        return E_FAIL;
    }
    return S_OK;
}

The internal Spot The Defect players found a good dozen or so defects -- some quite serious -- in this simple code.  How many can you find?

  • You got it. Though the caveat "16 bit versions only" could use a whole lot more supporting text than is given in the documentation. Does it mean that in 32 bit versions, both may be filled in, or neither may be filled in, or what?

    Most callees fill in the scode and zero the wcode, but given how confusing this is, you should not rely on that. Zero out both of them before the EXCEPINFO goes in, and check both of them when it comes back.

    That is a flaw but it is not the most serious flaw in this code.
  • The code assumes that the base type of the array is 32-bit integer, but the caller may have supplied something else.
  • Indeed -- another assumption that should be either validated by a check or assertion. Or, the type of the array could be read out of the array itself, which would make this method more general-purpose.
  • It can leak BSTRs from EXCEPINFO. And if you fix this, they need to be initialized to zero.
  • You got it. That's the big one, aside from the VT_ARRAY thing. This thing is as leaky as an unstanched wench, whatever that means. (Thank you, Will Shakespeare.)

    And yes, it's a good idea to not rely on the callee to null out the strings for you, though it should.

    There are other minor problems with this code, but that's most of them. Anyone see any others?


  • Isn't the returnVal method eating other valid success codes.
  • It sure is! See the next day's entry for some more.
Page 2 of 2 (22 items) 12