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?

  • I think you are missing a

    memset(params,0,sizeof(DISPPARAMS));


  • 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?
  • Check the input pointers - they could be null
  • 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.
  • Call VariantInit on *args
    check (param1!=0) before dereferencing it

    <macro valu=evil>
    and s/NULL/0
    </macro>
  • 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?
  • 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.
  • if (DISP_E_EXCEPTION == invokeHr) && SUCCEEDED(scodeHR) then failures will be silently translated into apparent successes.
  • > 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.
  • > 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?

  • > 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.)
  • > 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?
  • 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?
  • > 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?
  • > 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.
Page 1 of 2 (22 items) 12