Spot the Defect, Part Two

About Eric Lippert
Eric Lippert

Eric Lippert is a principal developer on the C# compiler team. Learn more about Eric.

Menu
Recent Posts
Live Now on Developer Tools Blogs
Tags
More ▼
Related resources
Archives
Archives
More ▼

Spot the Defect, Part Two

16 Aug 2004 9:01 AM
  • 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?

,
  • memset guy
    16 Aug 2004 10:05 AM
    I think you are missing a

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


  • Eric Lippert
    16 Aug 2004 10:08 AM
    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?
  • Sriram
    16 Aug 2004 10:11 AM
    Check the input pointers - they could be null
  • Eric Lippert
    16 Aug 2004 10:15 AM
    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.
  • Vatsan Madhavan
    16 Aug 2004 10:31 AM
    Call VariantInit on *args
    check (param1!=0) before dereferencing it

    <macro valu=evil>
    and s/NULL/0
    </macro>
  • Eric Lippert
    16 Aug 2004 10:34 AM
    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?
  • Curt Hagenlocher
    16 Aug 2004 10:52 AM
    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.
  • Carlos
    16 Aug 2004 10:56 AM
    if (DISP_E_EXCEPTION == invokeHr) && SUCCEEDED(scodeHR) then failures will be silently translated into apparent successes.
  • Eric Lippert
    16 Aug 2004 10:59 AM
    > 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.
  • Eric Lippert
    16 Aug 2004 11:02 AM
    > 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?

  • Eric Lippert
    16 Aug 2004 11:04 AM
    > 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.)
  • Curt Hagenlocher
    16 Aug 2004 11:08 AM
    > 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?
  • Eric Lippert
    16 Aug 2004 11:15 AM
    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?
  • Curt Hagenlocher
    16 Aug 2004 11:24 AM
    > 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?
  • Carlos
    16 Aug 2004 11:26 AM
    > 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