Spot the Defect, Part Two

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?

Comments

• Anonymous
  August 16, 2004
  I think you are missing a
  
  memset(params,0,sizeof(DISPPARAMS));
  
  
  
• Anonymous
  August 16, 2004
  Since cNamedArgs is set to zero it is not necessary to set the other field to null. But it would be a good programming practice to either initialize every member explictly or set the whole thing to zero first.
  
  What else?
• Anonymous
  August 16, 2004
  Check the input pointers - they could be null
• Anonymous
  August 16, 2004
  Indeed -- some asserts would be a good idea if this is an internal method, and some checks for null that return E_POINTER would be good if this is a public method.
  
  Since it is called "invokeHelper" odds are good that this is an internal method and could therefore benefit from some asserts.
  
• Anonymous
  August 16, 2004
  Call VariantInit on *args
  check (param1!=0) before dereferencing it
  
  <macro valu=evil>
  and s/NULL/0
  </macro>
• Anonymous
  August 16, 2004
  There's no need to call VariantInit on args because all VariantInit does is sets vt to VT_EMPTY, and vt is being set here.
  
  And yes, as the previous commenter pointed out, asserting or checking that the passed-in parameter is valid is a good idea.
  
  Anything else seem odd about param1?
• Anonymous
  August 16, 2004
  The VT_SAFEARRAY should be a VT_ARRAY. What's odd about param1 is that it's a SAFEARRAY**, yet the code only ever uses it as a SAFEARRAY*. The signature of InvokeHelper suggests that maybe it should be treated as in in/out parameter.
  
  hrInfo should be cleared to zero.
• Anonymous
  August 16, 2004
  if (DISP_E_EXCEPTION == invokeHr) && SUCCEEDED(scodeHR) then failures will be silently translated into apparent successes.
• Anonymous
  August 16, 2004
  > What's odd about param1 is that it's a SAFEARRAY**, yet the code only ever uses it as a SAFEARRAY*
  
  That is pretty weird! It looks like an in-out, but it isn't. Again, the code actually works but it is certainly a misleading design.
  
  > The VT_SAFEARRAY should be a VT_ARRAY
  
  Congratulations, this is the first actual code-doesn't-work defect found.
  
  VT_SAFEARRAY is intended to be used in the TYPEDESC structure, not the DISPPARAMS structure, and it cannot be OR'd with other values.
  
• Anonymous
  August 16, 2004
  > if (DISP_E_EXCEPTION == invokeHr) && SUCCEEDED(scodeHR)
  
  Yes, but you need more analysis.
  
  Why would a callee return an exception with the scode set to a success code? That seems really bizarre, but there is one scenario in which it happens. What is it?
  
  
• Anonymous
  August 16, 2004
  > hrInfo should be cleared to zero
  
  Yes, it would be a good programming practice to set the excepinfo to all zeros. It is not strictly necessary, but it is a good defensive programming technique.
  
  (The OLE Automation Programmer's Reference does NOT do so on page 142, oddly enough.)
• Anonymous
  August 16, 2004
  > Why would a callee return an exception with the scode set to a success code?
  
  Well for one thing, the callee could have badly written code... . But more specifically, aren't there several cross-apartment marshaling scenarios where the exception info is lost? Maybe cross-machine?
• Anonymous
  August 16, 2004
  Well, if the callee is so badly written that it returns a "success exception" then one could make the argument that the right thing to do is to eat the error!
  
  I'm unaware of any marshaling issues with EXCEPINFOs that lose error information.
  
  There is a far less bizarre, perfectly legal situation in which the callee returns an EXCEPINFO with the scode set to success. (I missed it too the first time I saw this question, and I've written LOTS of error handling code.) What is it?
• Anonymous
  August 16, 2004
  > There is a far less bizarre, perfectly legal situation in which the callee returns an EXCEPINFO with the scode set to success.
  
  The documentation for EXCEPINFO SAYS "Either this field or wCode (but not both) must be filled in; the other must be set to 0. (16-bit versions only)" Is that what you have in mind?
• Anonymous
  August 16, 2004
  > Why would a callee return an exception with the scode set to a success code?
  
  A 16-bit SCODE can be returned in the wCode field, in which case the scode field will be zero.
  
• Anonymous
  August 16, 2004
  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.
• Anonymous
  August 16, 2004
  The code assumes that the base type of the array is 32-bit integer, but the caller may have supplied something else.
• Anonymous
  August 16, 2004
  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.
  
• Anonymous
  August 16, 2004
  It can leak BSTRs from EXCEPINFO. And if you fix this, they need to be initialized to zero.
• Anonymous
  August 16, 2004
  The comment has been removed
• Anonymous
  August 18, 2004
  Isn't the returnVal method eating other valid success codes.
• Anonymous
  August 19, 2004
  It sure is! See the next day's entry for some more.