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 (22)

  1. memset guy says:

    I think you are missing a

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

  2. Eric Lippert says:

    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?

  3. Sriram says:

    Check the input pointers – they could be null

  4. Eric Lippert says:

    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.

  5. Vatsan Madhavan says:

    Call VariantInit on *args

    check (param1!=0) before dereferencing it

    <macro valu=evil>

    and s/NULL/0

    </macro>

  6. Eric Lippert says:

    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?

  7. Curt Hagenlocher says:

    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.

  8. Carlos says:

    if (DISP_E_EXCEPTION == invokeHr) && SUCCEEDED(scodeHR) then failures will be silently translated into apparent successes.

  9. Eric Lippert says:

    > 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.

  10. Eric Lippert says:

    > 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?

  11. Eric Lippert says:

    > 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.)

  12. Curt Hagenlocher says:

    > 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?

  13. Eric Lippert says:

    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?

  14. Curt Hagenlocher says:

    > 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?

  15. Carlos says:

    > 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.

  16. Eric Lippert says:

    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.

  17. Carlos says:

    The code assumes that the base type of the array is 32-bit integer, but the caller may have supplied something else.

  18. Eric Lippert says:

    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.

  19. Carlos says:

    It can leak BSTRs from EXCEPINFO. And if you fix this, they need to be initialized to zero.

  20. Eric Lippert says:

    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?

  21. Terry Denham says:

    Isn’t the returnVal method eating other valid success codes.

  22. Eric Lippert says:

    It sure is! See the next day’s entry for some more.