Platform SDK Shell, Another subtle boo-boo that'll bite ya a good one....

I had this nice little CQueryInfo class that was implementing the methods of
IQueryInfo so that my NSE could provide InfoTips when the mouse pointer
hovered over an item in the shell. The problem is, this CQueryInfo class
was lacking something subtle yet important... which, namely, was the ":
public IQueryInfo" after the declaration of the class "class CQueryInfo".

What this leads to is construction by the compiler of a vtable in which the
2 methods of the interface just happen to be out of order from what the
shell expects them to be. The end result is that the code will compile &
link, and it'll even be executed and you can debug into it. The problem is
that the wrong methods are getting called due to the botched vtable, and the
end result is stack corruption after a method call, and that's only if
you're really lucky. If you're not so lucky, then bogus data may get
returned or the type of stack corruption that occurs doesn't lead to an
immediate crash, but at a later time when your NSE's code isn't on the call
stack at all, the corruption will have been propagated in some way such that
the explorer ends up giving an accvio or some other GPF for no apparent reason.

This same issue can strike if you get hasty and add an interface to an
existing class and you fail to add the appropriate inheritance clauses to
the class declaration.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Sounds like you copied a bad implementation of QueryInterface from a sample such as SampView. You should use static_cast when copying the "this" pointer to the out-parameter. That way, you will get a compiler error if the class doesn't derive from the interface being implemented. ATL interface maps automatically give protection against against this programming error.

--
Jim Barry, MVP for Windows SDK

Yes, some of the code is modeled on what's present in both SampView and
CDBView, and I've learned that both of those examples are both "old" and buggy.

As for using "static_cast<>", it seems that it doesn't really do what you're
suggesting it should do. If I cast a base class pointer to a derived class
even though the pointer isn't for an object that really is of a derived
class, the cast operation will succeed but the object actually pointed to
isn't really valid and some sort of error is going to occur during member or
method access.

Perhaps dynamic_cast<> would be safer, although it imposes more overhead.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Chuck, I don't quite know what you thought I was suggesting, so I'll spell it out with an example. The code in SampView goes something like this:

STDMETHODIMP CShellFolder::QueryInterface(REFIID riid, LPVOID *ppReturn)
{
*ppReturn = NULL;

//IUnknown
if(IsEqualIID(riid, IID_IUnknown))
{
*ppReturn = this;
}

//IShellFolder
else if(IsEqualIID(riid, IID_IShellFolder))
{
*ppReturn = (IShellFolder*)this;
}

// ...
}

The problem is that the C-style cast is a sledgehammer - it will happily coerce one pointer type to any other pointer type. So, as you discovered, the code still compiles even if CShellFolder does not actually derive from IShellFolder.

By contrast, the C++ static_cast operator is a much more precise tool. It is basically the inverse of an implicit cast. If type A implicitly converts to type B, then static_cast provides a conversion from type B to type A. So we can rewrite the above implementation as:

STDMETHODIMP CShellFolder::QueryInterface(REFIID riid, LPVOID *ppReturn)
{
*ppReturn = NULL;

//IUnknown
if(IsEqualIID(riid, IID_IUnknown))
{
*ppReturn = static_cast<IShellFolder*>(this);
}

//IShellFolder
else if(IsEqualIID(riid, IID_IShellFolder))
{
*ppReturn = static_cast<IShellFolder*>(this);
}

// ...
}

Now the code only compiles if CShellFolder indeed derives from IShellFolder, otherwise it is ill-formed (giving error C2440 with the Microsoft compiler). Note that because every interface derives from IUnknown, there is an abiguity when casting directly from CShellFolder* to IUnknown* (here it could be the IUnknown inherited via either IShellFolder or IPersistFolder). The solution is simply to pick one of the directly-inherited interfaces - in this case I chose IShellFolder but it really doesn't matter which it is.


There's no need for that.

--
Jim Barry, MVP for Windows SDK

I understand what you're suggesting, but what I read about the static_cast<>
operator in the online help didn't leave me with the same understanding.
I'm re-reading it againg just to clarify my understanding of what I'm
reading in the docs. I will update my code to use static_cast<> as part of
following "best practices" in coding techniques.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Looking at the rest of the code that is part of the typical QueryInterface()
method implementation and we come to the typecast involved in calling the
AddRef() method on the interface pointer, shouldn't that also be changed
over to use static_cast<> as well?

if(*ppReturn)
{
(*(LPUNKNOWN*)ppReturn)->AddRef();
return S_OK;
}

would become:

if(*ppReturn)
{
(*static_cast<LPUNKNOWN*>ppReturn)->AddRef();
return S_OK;
}


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Yes, though it doesn't really add any benefits. Actually, as long as you're not doing any fun stuff like aggregation or tear-off interfaces, you can just call AddRef directly via the implicit "this" pointer.


Alternatively, to eliminate the cast, you could use an intermediate variable, something like this:

STDMETHODIMP CShellFolder::QueryInterface(REFIID riid, LPVOID *ppReturn)
{
IUnknown* result = NULL;

// IUnknown
if (IsEqualIID(riid, IID_IUnknown))
{
result = static_cast<IShellFolder*>(this);
}
// blah blah blah

*ppReturn = result;
if (result)
{
result->AddRef();
return S_OK;
}
return E_NOINTERFACE;
}

--
Jim Barry, MVP for Windows SDK