Memory leak with CComPtr<T>::operator&
Because CComPtrBase<T>::operator& / CComPtr<T>::operator& is not freeing the pointer, a memory leak can ensue, when handing out the pointer twice (the example is rather silly, but shows the point):
IMAPISession *ses = gimme();
CComPtr<IUnknown> arc;
if (ses->OpenMsgStore(0, eid_size, eid, &IID_IUnknown, 0, &arc) == hrSuccess) {
/* Overwrites arc without releasing the first store instance */
ses->OpenMsgStore(0, eid_size, eid, &IID_IUnknown, 0, &arc);
}
Kopano's mapi4linux fixes this by forcing the developer to clear the pointer a priori, by prohibiting the (direct) use of object_ptr::operator& on the smart pointer. There is, instead, a object_ptr::operator~ which returns a proxy class that first releases the pointer and then provides the operator& function.
A few functions, like GetIDsFromNames, expect a pointer with "inout" semantics. For that, unary object_ptr::operator+ is used. This is equivalent to the no-release operator&. The uses of inout params is few, and "&+" will stand out in source code as intended.
/* class object_ptr { void operator&() = delete; } */
KC::object_ptr<IUnknown> arc;
/* Intentionally won't compile */
ses->OpenMsgStore(0, eid_size, eid, &IID_IUnknown, 0, &arc);
/* Clears arc before the call */
ses->OpenMsgStore(0, eid_size, eid, &IID_IUnknown, 0, &~arc);
/* GetIDsFromNames second parameter is "inout" */
blah->GetIDsFromNames(1, &+lpNameID, ulFlags, &~ptrPropTags);
The problem of CComPtr<T>::operator& is replicated in other classes' operator&, such as CHeapPtr<T>::operator&.
OpenEntry param #5 should have been void **
struct IMAPISession {
HRESULT OpenEntry(ULONG cbEntryID, LPENTRYID lpEntryID, LPCIID lpInterface, ULONG ulFlags, ULONG FAR *lpulObjType, LPUNKNOWN FAR *lppUnk);
…
};
The OpenEntry function of classes like IMAPISession and others takes a final argument of type IUnknown ** where it will return a pointer to arbitary type. However, only void * (more specifically, void far * in the context of x86 16-bit programming) is guaranteed to be able to hold all kinds of pointers, so void ** should have been used as the 5th argument type instead of IUnknown **. The QueryInterface function got it right.
OpenEntry with NULL IID argument is broken by design
The IMAPISession::OpenEntry function, when called with a NULL/nullptr IID argument leads to a haphazard situation where you either spring a memory leak or invoke undefined behavior.
OpenEntry yields a pointer to the newly-opened object in the 5th argument. The 3rd argument given to it specifies the pointer type that the programmer desires, in this case, it is an IMessage for example:
IMsgStore *store = gimme();
IUnknown *ptr;
store->OpenEntry(eidsize, eid, &IID_IMessage, &type, &ptr);
auto msg = reinterpret_cast<IMessage *>(ptr);
msg->Release();
The ptr variable serves is just there “for transport”. The object it points to is really an IMessage (as requested), and this is why we have to cast back to the true type before being able to use it.
When passing NULL for the IID, the function is free to return any pointer type it seems fit, and it will indicate which pointer type it was in the type variable. If, however, type is not a value recognized by the program, you have a problem.
IMsgStore *store = gimme();
IUnknown *ptr = nullptr;
unsigned int type = 0;
store->OpenEntry(eidsize, eid, nullptr, &type, &ptr);
if (type == MAIL_MESSAGE) {
auto msg = reinterpret_cast<IMessage *>(ptr);
msg->do_stuff();
msg->Release();
} else {
/* what now? Release()? No. */
}
In the “else” block, calling ptr->Release() to dispose of the object can invoke undefined behavior. Consider type == MAPI_FOLDER, in which case ptr is really a pointer to an IMAPIFolder object.
Avoiding the Release() call avoids UB, but leads to a leak.