A colleague pointed out that code snippet in previous post is misusing ATL’s ATLENSURE_SUCCEEDED macro making it [possibly] evaluate its argument twice in case of failure, that is evaluating into failure HRESULT code. As it is defined like this:
#define ATLENSURE_SUCCEEDED(hr) ATLENSURE_THROW(SUCCEEDED(hr), hr)
It does things in a straightforward way, for a code line
ATLENSURE_SUCCEEDED(pFilterGraph.CoCreateInstance(CLSID_FilterGraph));
It is doing “let’s CoCreateInstance the thing and if it fails, let’s CoCreateInstance it again to find out error code”. Disassembly shows this clearly:
This is exactly another spin of the story previously happened with HRESULT_FROM_WIN32 macro and possibly a number of others. With it being originally a macro, SDK offered an option to override the definition by pre-defining INLINE_HRESULT_FROM_WIN32. This way a user might be explicitly requesting a safer definition while still leaving legacy code live with macro. See more detailed story on this in Matthew’s blog.
A tricky thing is that with successful execution the problem does not come up. In case of failure, it depends on the functions called, some with just repeat the error code, some will return a different code on second run, some might create less desired and expected consequences. So you can find yourself having written quite some code before you even suspect a problem.
Having identified the issue, there are a few solutions.
1. First of all, the original ATLENSURE_SUCCEEDED macro can still be used, provided that you don’t put expressions as arguments.
This is going to do just fine:
const HRESULT nCoCreateInstanceResult = pFilterGraph.CoCreateInstance(CLSID_FilterGraph); ATLENSURE_SUCCEEDED(nCoCreateInstanceResult);
2. Second straightforward way is to replace the original ATL definition in ATL code (boo, woodenly)
3. As ATL code is checking for the macros to be already defined, and skipping its own definition in such case, it is possible to inject a safer private definition before including ATL headers (which would typically need one to do the define in stdafx.h):
#define ATLENSURE_SUCCEEDED(x){ const HRESULT nResult =(x); ATLENSURE_THROW(SUCCEEDED(nResult), nResult); } #include <atlbase.h> #include <atlstr.h>
Pre-evaluating the argument into local variable is going to resolve the original multi-evaluation problem.
4. There might be a new inline function defined on top of the original macro, which will be used instead and which is free from the problem:
inline VOID ATLENSURE_INLINE_SUCCEEDED(HRESULT nResult) { ATLENSURE_SUCCEEDED(nResult); }
Either way, the correct code compiles into single argument evaluation and throws an exception with failure code immediately:
Also, vote for the suggestion on Microsoft Connect. The issue is marked as fixed in future version of Visual Studio.
Recent Comments