Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 14

What's wrong with this code, part 14

  • Comments 32
Keeping up with the "theme" of COM related bad code examples, here's another real-world example.  To avoid any ATL confusion, it's 100% pure C++.

Our test team rolled out a new set of tests last week and immediately hit this one.  The funny thing is that the code in question had worked  without flaw for two years before this.

Obviously this has been abstracted to the point of ridiculousness, there's just enough here to demonstrate the bug...

struct FooConfig
{
    int _Value1;
    int _Value2;
};

[
    object,
    uuid("0A0DDEDC-C422-4BB3-9869-4FED020B66C5"),
    pointer_default(unique)
]
__interface IFoo : IUnknown
{
    HRESULT GetFooConfig([out] struct FooConfig **ReturnedFooConfig);
};

FooConfig _GlobalFooConfig = { 1, 2};

class CFoo: public IFoo
{
    LONG _refCount;
public:
    CFoo() : _refCount(1) {};

    // IFoo
    HRESULT GetFooConfig(FooConfig **ReturnedFooConfig)
    {
        *ReturnedFooConfig = &_GlobalFooConfig;
        return S_OK;

    }

    // IUnknown
    virtual HRESULT STDMETHODCALLTYPE QueryInterface(const IID& iid, void** ppUnk)
    {
        HRESULT hr=S_OK;
        *ppUnk = NULL;
        if (iid == __uuidof(IFoo))
        {
            AddRef();
            *ppUnk = reinterpret_cast<void *>(static_cast<IFoo *>(this));
        }
        else if (iid == IID_IUnknown)
        {
            AddRef();
            *ppUnk = reinterpret_cast<void *>(static_cast<IUnknown *>(this));
        }
        else
        {
            hr = E_NOINTERFACE;
        }
        return hr;
    }
    virtual ULONG STDMETHODCALLTYPE AddRef(void)
    {
        return InterlockedIncrement(&_refCount);
    }
    virtual ULONG STDMETHODCALLTYPE Release(void)
    {
        LONG refCount;
        refCount = InterlockedDecrement(&_refCount);
        if (refCount == 0)
        {
            delete this;
        }
        return refCount;
    }
};

This one's pretty straighforward, and I expect that people will see the problem right away.  So I'm going to raise the bar a bit - to get full credit, you not only have to explain not only what the problem is, but also why we'd never seen a problem with this code before.

And, as always, kudos and mea culpas on Monday.

Edit: Fixed return value from GetFooConfig.

  • GetFooConfig is missing a 'return S_OK', and I'm guessing the reason it isn't noticed is that typically QueryInterface is called first then GetFooConfig, and since by convention return values are stored in the eax register on x86, QueryInterface's returned value of S_OK is retained as GetFooConfig's return value as well. Correct?
  • bao, no, that's not it (but thanks for picking it up). For some reason the C compiler didn't complain (although I'm not sure why).

  • Gee... I guess it has to do with GetFooConfig returning a pointer to that global variable. What happens if you try to marshal that call on a different process? Won't the stub assume the memory has been dynamically allocate and try a CoTaskMemFree on it?
  • ReturnedFooConfig should be allocated using CoTaskMemAlloc and returned. It should be a copy of the global variable. The error will be observed only when the calling application retains the value returned by the function beyond the time the inproc server is loaded in memory. It will also be observed when the call is marshaled in that case COM Runtime will try to free the returned pointer using CoTaskmemFree. If the call is within the same apartment and the caller doesnot reatin the pointer beyond the lifetime of the dll, and he doesnot free the returned memory using CoTaskMemFree (which legaly he should) the problem will not be observed.
  • ThalesC got the problem, but not the answer.

    vrk also added in a reason. But why didn't we find the problem, and what's different about the test case?
  • In the case somebody call GetFooConfig with a NULL we will se a access violation.
    The method should contain a check:

    if (!ReturnedFooConfig)
    return E_POINTER;

    CoTaskMemFree can fail silent if you use a wrong pointer (not allocate with CoTaskMemAlloc). Maybe the new test case contains a custom memory allocator.
  • hey larry, post some C# code ;)

    btw when do MS devs comment their code ..after or while they are coding???

    why don't I see comments?
  • Is the test loading the class as a DLL? Once the DLL is unloaded globals defined in it will go away - thus the returned FooConfig pointer will no longer be valid.

    So if they do something like:
    h = LoadLibrary("MyTest");
    p = new CFoo();
    p->GetFooConfig(&r);
    FreeLibrary(h);
    r->_Value1 = 0;

    then the test will asplode.
  • Larry, perhaps I'm being pedantic, but that's not 100% pure C++. Sure, it's 100% pure Microsoft C++ as of 7.0.
  • Orbit, I've done C# code in bad code examples in the past. But the problem doesn't show up easily with C# (since COM's implemented via interop).

    And the reason you don't see comments is that there's a lot of code in the example already.

    Aaron: This code is never accessed by any mechanism other than CoCreateInstance - so nobody's directly instantiating a CFoo
  • are you still on the media team?
  • Alex, you're right, but the actual code is clean as far as I know - I could have broken the interface and struct into their own .idl file instead of using the extensions but it was easier this way.
  • Oh, and Orbit, I'm still on windows core audio (which is part of the Media Technologies Group (mediatech), which is part of the Digital Media Division, which is a part of...)
  • I suspect that the old tests were using the COM object inproc. Hence they didnt hit this issue. THe new tests might be using this object out of proc?
  • which is part of Windows Media Player? one question, I'm running Windows XP home sp2, When I exit WMP the song still runs in the background, looks like it doesn't exit so I have to kill the process? known bug or is it just me?

    plus I'm getting tired of the same 3 languages..VB,C#,C++ ..C# is basically a Java copy but you think we will see a totally new language being implemented for a niche part of developers?

    I read blogs that VB and C# have the same capability but C# gets the better sterotype because it has the "letter" C which people think of the power of the C languages.

    any ideas? (Comega and F# are just extensions to the C# language)
Page 1 of 3 (32 items) 123