Re: Patch: Wrong ownership sematics for BSTRs

In this patch - one more occurence of BSTR leakage and wrong handling of IUnknown::QI() in class factories creation.

What I find very confusing is checking the pointer returned by "new" for being null. See this SO question: http://stackoverflow.com/questions/550451/will-new-return-null-in-any-case Basically unless you take very special measures usual "new" will never return null pointers but instead will throw std::bad_alloc.

Since the code in question is a COM method it is not allowed to let exceptions propagate through the COM boundary. So in current implementation if "new" ever runs out of memory an exception is thrown that is propagated to the consumer and causes undefined behaviour there - most likely the consumer will crash.

That's no so bad by itself, but the very same behaviour would be there even without any checks. So current implementation only looks reliable - it checks for null to return E_OUTOFMEMORY which will just never happen, so that checking code is currently useless and only inflates the codebase.

There're two possible solutions. Either "std::nothrow new" can be used - it will return null in case of no memory available. Or some construct for catching all exceptions can be used - something like _ATLTRY _ATLCATCHALL macros used everywhere in ATL in such cases. IMO the first variant (nothrow new) is better - cleaner, more straitforward and less changes. What's your opinion?

Best wishes.
Dmitry.

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=2437434

To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].