Compartilhar via

Solution to Spot the Defect Part Two

  • Artigo

There were l

ots of good responses to my challenge of yesterday. There are two major defects in this code: that it leaks memory, and that it passes an incorrect variant type to Invoke. There are also a considerable number of minor defects and questionable design decisions. Let's go through that code a line at a time and see what could be better.

HRESULT CInvokeHelper::InvokeHelper(IDispatch *pDisp, long dispid, SAFEARRAY **param1)
{

1) There should be an assertion or runtime check that pDisp is a pointer to good memory.
2) I would have typed the dispid parameter as DISPID, not long. Same thing ultimately, but you know, we have these abstractions for a reason.
3) *param1 is only ever read, not written. There's no reason for it to be a SAFEARRAY**. It could be a SAFEARRAY* just as well. Couldn't hurt to put some assertions on there as well.

non-defect: param1 looks like a badly named parameter, but it actually makes some sense, as it will be the first (and only) parameter to the late-bound call.

    HRESULT hr;
DISPPARAMS params;

4) Defensive programming: params should be fully initialized, either with a memset, or by setting the named arguments array to NULL.

    EXCEPINFO hrInfo;

5) hrInfo is a lousy name for an EXCEPINFO structure. Why not excepinfo?
6) Defensive programming: clear all the fields.

    VARIANTARG args[1];

non-defect: no need to initialize this with VariantInit, as it's vt is going to be set in just a moment.

    params.cArgs=1;
params.rgvarg=args;
params.cNamedArgs=0;
params.rgvarg[0].vt=VT_SAFEARRAY | VT_I4;

7) VT_SAFEARRAY should be VT_ARRAY

    params.rgvarg[0].parray = *param1;
hr = pDisp->Invoke(dispid,IID_NULL, LOCALE_USER_DEFAULT,

8) LOCALE_USER_DEFAULT isn't necessarily the best choice here. Most callees ignore it, but there are potential gotchas. I usually set it to 0x0409, US-English.

       DISPATCH_METHOD, &params, NULL, &hrInfo, NULL);
return returnVal(hr, hrInfo.scode);

9) The callee is not guaranteed to set the scode to anything -- wcode could be set instead, and then scode would be S_OK. Then the error info is lost! This thing should be passing in the whole EXCEPINFO, not just one part of it.

10) If the callee returned strings in the EXCEPINFO, they’ve just leaked memory.

}

HRESULT CInvokeHelper::returnVal(HRESULT invokeHr, HRESULT scodeHR)
{
m_invokeResult = invokeHr;

11) OK, this is bizarre. It stashes away the returned HRESULT. What if the callee returned DISP_E_EXCEPTION and the EXCEPINFO contained E_OUTOFMEMORY? What do we store?  DISP_E_EXCEPTION, which tells you nothing. Why is this being stored? Hard to say without seeing the rest of the code, but it seems wrong.

    if(FAILED(invokeHr))
{
if(DISP_E_EXCEPTION == invokeHr)
return scodeHR;

As mentioned above, this could be S_OK but there is still an error situation.

return E_FAIL;

12) E_OUTOFMEMORY comes in, E_FAIL goes out, and if the error message ever gets to the user, they see "Something failed". The error has been preserved in the class state of course, but who knows what it does with it. Why lose the information in the first place? It seems clear that the purpose of this method is to extract error information from an EXCEPINFO, but what it actually does is changes how error information is stored and propagated depending on where the error information came from. That seems completely wrong -- you'd think you'd want to have a routine that makes how the error information is stored and propagated the same no matter where the error information came from.

}
return S_OK;

13) Again, this eats information. The method might have returned S_FALSE, but the caller will never know.

}

Clearly, testing error code paths is incredibly important. They're easy to get wrong.

Comments

  • Anonymous
    August 17, 2004
    Please could you expand on point 8, since whilst I understand your other points -- though I missed most of them, having avoided IDispatch -- this one is non obvious.

    I'll obviously exclude the possibility of biggotry since it comes from you and in a post about correctness, but is using 0x0409 a case of "Standard practice is to do this" (if so, why?) or simply "The code is unlikely/least likely to expect something else"? Am I reading too much into your contrasting of the symbol LOCALE_USER_DEFAULT with the integer literal 0x409? (Shamefully, I'm not nearly as knowledgable on the subject of internationalisation issues in programming as I'd like to be)
  • Anonymous
    August 17, 2004
    Indeed, you are correct that point #8 is a total hand wave.

    This is a surprisingly tricky issue with a bizarre history and an impact on VSTO. I'll write a blog entry on issues involving the locale parameters in OLE Automation at a later date, don't worry.
  • Anonymous
    August 17, 2004
    I'm confused by point #8 too. Handwave or not, the confusion remains. You assume that most of your program's users will want a combination of a foreign language (for the method that you invoke) together with (for everything else) a language that might be their native language or might just be the language that the rest of their OS operates in. Sure sometimes users want combinations like this, but why do you push it on everyone?

    By the way I almost said "all" of your users instead of "most". But then remembered that some of your users really do have systems where 0x0409 is the default language -- and this includes me, since Windows 2000 was the only version where English-language help files were downloadable. Sometimes I install English-language versions in order to read help files or do some experiments. But it's not as though this could be done in daily life.
  • Anonymous
    August 24, 2004
    The comment has been removed