Quiz: What's wrong with this code?
What's wrong with the following code? This is real code taken verbatim from Cordbg (see src\debug\shell\dshell.cpp in the rotor sources), except that I removed preprocessor goo to make it more readable. All the interfaces are COM-classic interfaces derived from IUnknown. You don't need to be familiar with ICorDebug to see the problem here (that's hint #1 - it's not related to some subtle API misuse), though I give some background data below for the curious. Here's a hint #2: Ever notice memory leaks in Cordbg?
//
// Strip all references off of the given value. This simply
// dereferences through references until it hits a non-reference
// value.
//
// On input *ppValue is an ICorDebugValue for some arbitrary variable.
// On output, *ppValue is update to yield an ICorDebugValue which is
// the furthest down the dereference chain.
//
// The caller requires no specific knowledge of what the *ppValue passed in is.
// The caller must have a reference to the incoming *ppValue (which this function will release)
// and must release the outgoing *ppValue (which this function would addref).
//
HRESULT DebuggerShell::StripReferences(ICorDebugValue **ppValue,
bool printAsYouGo)
{
HRESULT hr = S_OK;
while (TRUE)
{
ICorDebugReferenceValue *reference;
hr = (*ppValue)->QueryInterface(IID_ICorDebugReferenceValue,
(void **) &reference);
if (FAILED(hr))
{
hr = S_OK;
break;
}
// Check for NULL
BOOL isNull;
hr = reference->IsNull(&isNull);
if (FAILED(hr))
{
reference->Release();
(*ppValue)->Release();
*ppValue = NULL;
break;
}
if (isNull)
{
if (printAsYouGo)
Write(L"<null>");
reference->Release();
(*ppValue)->Release();
*ppValue = NULL;
break;
}
CORDB_ADDRESS realObjectPtr;
hr = reference->GetValue(&realObjectPtr);
if (FAILED(hr))
{
reference->Release();
(*ppValue)->Release();
*ppValue = NULL;
break;
}
// Dereference the thing...
ICorDebugValue *newValue;
hr = reference->Dereference(&newValue);
if (hr != S_OK)
{
if (printAsYouGo)
if (hr == CORDBG_E_BAD_REFERENCE_VALUE)
Write(L"<invalid reference: 0x%p>", realObjectPtr);
else if (hr == CORDBG_E_CLASS_NOT_LOADED)
Write(L"(0x%p) Note: CLR error -- referenced class "
L"not loaded.", realObjectPtr);
else if (hr == CORDBG_S_VALUE_POINTS_TO_VOID)
Write(L"0x%p", realObjectPtr);
(reference)->Release();
((*ppValue))->Release();
*ppValue = NULL;
break;
}
if (printAsYouGo)
Write(L"(0x%08x) ", realObjectPtr);
(reference)->Release();
((*ppValue))->Release();
*ppValue = newValue;
}
return hr;
}
Here's an explanation of the identifiers involved:
- ICorDebugValue is the debugger's representation of a variable.
- If that variable is a reference value, then it also implements ICorDebugReferenceValue.
- Here's what the following methods on ICorDebugReferenceValue mean:
- IsNull(out BOOL) - returns Boolean if the reference value in the debuggee is null.
- GetValue(out CORDB_ADDRESS) - get the address in the debuggee of what the reference points to.
- Dereference(out ICorDebugValue) - get an ICorDebugValue describing the variable that this reference points to.
Comments
- Anonymous
May 08, 2005
Without knowing the API, one potential problem is if Dereference returns a success value other than S_OK and sets newValue to something other than NULL. Then newValue would never be released. - Anonymous
May 08, 2005
The comment has been removed - Anonymous
May 08, 2005
What is the reference count of ppValue at the start of the function? - Anonymous
May 08, 2005
Sriram, Luke - both valid finds. And they're are still more.
Oshah - generally the only thing a function knows about the ref count of its incoming parameters is that they're some value greater than 1. This is actually related to a subtle 3rd problem in the code. - Anonymous
May 08, 2005
It seems that
*ppValue = NULL;
incorrectly assumes that the ppValue's reference count is exactly 1? - Anonymous
May 08, 2005
The (hr != S_OK) check could lead to a leak of ICorDebugValue references (if it succeeded in getting one but didn't return S_OK). Given that the documentation should list all success codes this may not be relevant.
There is no explanation given for the extra brackets wrapping reference and (*ppValue) in the later calls to Release().
A number of variables are declared without being initialised, which shouldn't have any effect but I'd try to avoid it.
There is no indication of whether realObjectPtr requires any kind of freeing. It either needs freeing or it's risky because it could provide a pointer to the internals of a non-existant object (though the code above does not appear to access it after reference has been Release()d.
An SEH anywhere in this code, or the code it calls, will leak references. If Dereference can return NULL (maybe in conjunction with an S_FALSE return code) this will happen when QueryInterface is called on the second run through the loop.
Getting anywhere? - Anonymous
May 08, 2005
Just thinking through Sriram's comment, the code should either Release *ppValue AND set ppValue to NULL OR it should do neither.
The caller is under instructions to Release any non-NULL pointer returned.
And if it did Release it then the caller would never get a successful result.
It's also worth noting that the function returns unknown successful HRs in two different places. It's documentation should either list all of the successful HRs from IsNull and Dereference, or it should explicitly set hr in these cases. - Anonymous
May 12, 2005
The comment has been removed - Anonymous
May 12, 2005
Surely that's not an issue.
If it has a refcount > 1 then something else (some other variable) should have a reference to it.
It is perfectly reasonable, and indeed, required, to set the variable to NULL after calling Release on it once - unless your code has caused it to increment refcount more than once.
Isn't it? - Anonymous
May 13, 2005
Yaytay: ppValue is passed to StripReferences() by reference. How can you possibly make any assumptions about its reference count in the StripReference() code?
An example from dshell.cpp:
ICorDebugValue *value = ...; // theoretically possible refcount > 1
...
GetStaticFieldValue(..., &value); // theoretically possible refcount > 1
...
StripReferences(&value, false); - Anonymous
May 13, 2005
My point is that you aren't making any assumptions about it's refcount.
The variable that you are dealing with is responsible for precisely one increment of that refcount, if the caller has allocated more than one increment to that variable then it is their issue to resolve it.
One variable is one reference.
If you choose to circumvent that by explicitly calling AddRef thn you are responsible for picking up the pieces. - Anonymous
May 14, 2005
The comment has been removed - Anonymous
May 14, 2005
Vladimir.
If StripReferences made the requirement that all callers make sure that ppValue has reference count 1, then the ppValue=NULL would not be an issue.
That's a very contrived situation though. In the same vein, we can make my scanf program work--if the user typed in the correct value of x each time. So all we need is to require the users to type the correct value of x; and my scanf program actually works!
Since ref counting is not mentioned anywhere in the code sample, we know this is clearly a bug. (However, I was not completely sure, hence why I asked for the exact reference count earlier). - Anonymous
May 15, 2005
OK, the common consensus appears to be that I am being particularly dense, but I'm afraid I'm still there.
StripReference does, explicitly, call Release before setting *ppValue to null.
Now it is certainly possible that the caller called (*ppValue)->AddRef() and didn't copy the pointer value, but that in itself would be ignoring the documentation as AddRef "should be called for every new copy of a pointer to an interface on a given object".
So either the refcount is 1 and calling (*ppValue)->Release invalidated the pointer, or the caller should have another variable with the same value as ppValue.
I'd certainly say that this sort of bare pointer work was dangerous, but it's dangerous because it permits the caller to do the wrong thing, not because it itself does a wrong thing. - Anonymous
May 17, 2005
The comment has been removed