{"id":102837,"date":"2019-09-04T07:00:00","date_gmt":"2019-09-04T14:00:00","guid":{"rendered":"http:\/\/devblogs.microsoft.com\/oldnewthing\/?p=102837"},"modified":"2019-09-03T18:51:30","modified_gmt":"2019-09-04T01:51:30","slug":"20190904-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20190904-00\/?p=102837","title":{"rendered":"The COM_<\/CODE>INTERFACE_<\/CODE>ENTRY<\/CODE> must be a COM interface, but nobody actually checks"},"content":{"rendered":"

A customer had some code written with the Active Template Library, more commonly known as ATL<\/a>. Apparently, ATL is still a thing!<\/p>\n

Anyway, their problem was that their component that had been working just fine for many years started crashing. Their object went something like this:<\/p>\n

[uuid(\"...\")]\r\nclass ATL_NO_VTABLE CAwesomeWidget :\r\n    public CComObjectRootEx<CComMultiThreadModel>,\r\n    public CComCoClass<CAwesomeWidget, &CLSID_AwesomeWidget>,\r\n    public IWidgetProviderInfo,\r\n    public IWidget\r\n    \/\/\/ ... other interfaces ...\r\n{\r\n    ...\r\n\r\n    BEGIN_COM_MAP(CAwesomeWidget)\r\n      COM_INTERFACE_ENTRY(IWidgetProviderInfo)\r\n      COM_INTERFACE_ENTRY(IWidget)\r\n      \/\/ ... other interfaces ...\r\n    END_COM_MAP()\r\n\r\n    \/\/ IWidgetProviderInfo\r\n    IFACEMETHODIMP GetProviderId(GUID *pguidId);\r\n\r\n    \/\/ IWidget\r\n    IFACEMETHODIMP Frob();\r\n    ...\r\n};\r\n<\/pre>\n
Yes, I’m making you look at ancient ATL code, with all its wacky macros. Product maintenance is like that.  Deal with it<\/a>.<\/p>\n
Clients can do<\/p>\n
void Sample()\r\n{\r\n  CComPtr<IWidget> widget;\r\n  widget.CoCreateInstance(CLSID_AwesomeWidget);\r\n  widget->Frob();\r\n}\r\n<\/pre>\n
to create the CAwesome\u00adWidget<\/code> as a widget, and then ask the widget to do something.<\/p>\n
The CAwesome\u00adWidget<\/code> did its job very well for over five years, and then suddenly it started crashing in Get\u00adProvider\u00adId<\/code>, even though nobody called Get\u00adProvider\u00adId<\/code>.<\/p>\n
They traced it back to this call:<\/p>\n
void Sample2()\r\n{\r\n  CComPtr<IUnknown> unk;\r\n  unk.CoCreateInstance(CLSID_AwesomeWidget);\r\n\r\n  CComQIPtr<IWidget> widget(unk); \/\/ \u2190 here<\/span>\r\n  widget->Frob();\r\n}\r\n<\/pre>\n
Obviously, this wasn’t literally what their code did, but it boils the problem down to its essence.<\/p>\n
The idea was that instead of creating the object for its IWidget<\/code> interface, the object was created with the generic IUnknown<\/code> interface, and then it was converted to an IWidget<\/code>. The reason for this extra step isn’t important, but you can come up with scenarios where this might happen. (For example, maybe the Co\u00adCreate\u00adInstance<\/code> is coming from a generic “creation helper” function.)<\/p>\n
The problem was in the definition of the IWidget\u00adProvider\u00adInfo<\/code>:<\/p>\n
\/\/ widget.h\r\n[uuid(\"...\")] \r\ninterface IWidgetProviderInfo\r\n{\r\n    STDMETHOD(GetProviderId)(GUID *pguidId) PURE;\r\n};\r\n<\/pre>\n
Do you see something?<\/p>\n
Actually, more honestly, I should be asking, “Do you not<\/i> see something?”<\/p>\n
The IWidget\u00adProvider\u00adInfo<\/code> interface does not derive from IUnknown<\/code>!<\/p>\n
Once you realize this, everything unravels.<\/p>\n
The COM_<\/code>INTERFACE_<\/code>ENTRY<\/code> macro assumes that the thing it’s given is indeed a COM interface. This assumption is made in two places:<\/p>\n
    \n
  1. A request for IUnknown<\/code> returns the first interface in the list.<\/li>\n
  2. Any successful request will be accompanied by a call to IUnknown::<\/code>Add\u00adRef<\/code>, per COM rules.<\/li>\n<\/ol>\n
    They listed IWidget\u00adProvider\u00adInfo<\/code> as the first “interface”, so it was returned as in response to a request for IUnknown<\/code>, even though it wasn’t IUnknown<\/code>.<\/p>\n
    The attempt to convert the IUnknown<\/code> to an IWidget<\/code> involves a call to IUnknown::<\/code>Query\u00adInterface<\/code>, but remember, that thing which the code thinks is an IUnknown<\/code> is really an IWidget\u00adProvider\u00adInfo<\/code>. The call to IUnknown::<\/code>Query\u00adInterface<\/code> actually called IWidget\u00adProvider\u00adInfo::<\/code>Get\u00adProviderId<\/code>. With the wrong number and types of arguments, of course. The crash occurred when the Get\u00adProvider\u00adId<\/code> method tried to write to what it thought was a pguidId<\/code>, but which was actually a riid<\/code> from the Query\u00adInterface<\/code>.<\/p>\n
    The customer would have noticed that something was wrong with IWidget\u00adProvider\u00adInfo<\/code> had they ever tried to use it!<\/p>\n
    CComPtr<IWidgetProviderInfo> info;\r\n\/\/ ^^ error: class \"IWidgetProviderInfo\" has no \"Release\" member\r\n<\/pre>\n
    Merely talking about IWidget\u00adProvider\u00adInfo<\/code> causes the compiler to get upset because the CComPtr<\/code> destructor needs to call the Release<\/code> method, which doesn’t exist. The customer never noticed this because they never used the IWidget\u00adProvider\u00adInfo<\/code> interface at all! It was presumably added in anticipation of a feature that never materialized.<\/p>\n
    Okay, so now we understand why this crashes and how it eluded compile-time detection, but how did it ever work? After all, the implementation of Create\u00adInstance<\/code> needs to do a Query\u00adInterface<\/code> to return the proper pointer back to the caller.<\/p>\n
    Here’s a simplified version of how ATL implements Create\u00adInstance<\/code>:<\/p>\n
    template <class T1>\r\nclass CComCreator\r\n{\r\n  static HRESULT WINAPI CreateInstance(void* pv, REFIID riid, LPVOID* ppv)\r\n  {\r\n    HRESULT hRes = E_OUTOFMEMORY;\r\n    T1* p = NULL;\r\n    ATLTRY(p = new T1(pv))\r\n    if (p != NULL)\r\n    {\r\n      p->SetVoid(pv);\r\n      p->InternalFinalConstructAddRef();\r\n      hRes = p->_AtlInitialConstruct();\r\n      if (SUCCEEDED(hRes))\r\n        hRes = p->FinalConstruct();\r\n      p->InternalFinalConstructRelease();\r\n      if (hRes == S_OK)\r\n        hRes = p->QueryInterface(riid, ppv);<\/span>\r\n      if (hRes != S_OK)\r\n        delete p;\r\n    }\r\n    return hRes;\r\n  }\r\n}\r\n<\/pre>\n
    Why doesn’t the call to p->Query\u00adInterface<\/code> crash?<\/p>\n
    Because it’s being called from a pointer to a T1<\/code>, not from a pointer to an IUnknown<\/code>. The compiler therefore knows that the Query\u00adInterface<\/code> method is in fact not at slot 0 in vtable 0, but rather is at slot 0 in vtable 1 (taking it from IWidget<\/code>).<\/p>\n
    So how about fixing the problem?<\/p>\n
    One fix is to delete the unused IWidget\u00adProvider\u00adInfo<\/code> interface. However, when working with legacy code, you may be averse to taking such a drastic step, because there might be somebody who is actually using that interface in a way you failed to detect. Or maybe you want to keep the interface around because you really do plan on using it soon. In that case, you can make the interface derive from IUnknown<\/code>, like it should have in the first place:<\/p>\n
    [uuid(\"...\")] \r\ninterface IWidgetProviderInfo : IUnknown<\/span>\r\n{\r\n    STDMETHOD(GetProviderId)(GUID *pguidId) PURE;\r\n};\r\n<\/pre>\n
    Customer problem solved.<\/p>\n
    Next time, we’ll look at how to catch this problem at compile time.<\/p>\n","protected":false},"excerpt":{"rendered":"
    And if it isn’t, bad things happen.<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-102837","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
    And if it isn’t, bad things happen.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/102837","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=102837"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/102837\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=102837"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=102837"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=102837"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}