1900964 - [meta] Crash in [@ RtlpFreeHeapInternal | RtlFreeHeap | APP_DATA::FreeCachedMem] - Heap corruption after double-freeing a BSTR

Status: NEW

(Firefox :: General, defect)

Description

[Yannis Juglaret [:yannis]]

The attached crash signature groups together various issues. The issues can be separated through aggregation on proto signatures. The most recurring way to end up in this crash signature (and the only way so far I believe) is that some code frees the same BSTR twice. We have seen this mistake occur in third-party DLLs (e.g. bug 1692908, bug 1900796) but also in Microsoft DLLs (e.g. bug 1675760 comment 11).

One reason for these bugs is that VariantClear on a VT_BSTR will free the string and sometimes people forget that. Another reason is that VariantClear only clears vt but it will not clear bstrVal.

Here are examples of what the faulty codes can look like:

BSTR bstr = SysAllocString(L"foo");

// ...

VARIANT var;
VariantInit(&var);

var.vt = VT_BSTR;
var.bstrVal = bstr;
// This is not a copy. This should be considered a transfer of ownership of the string from bstr to var.
// To do a copy here, use: var.bstrVal = SysAllocString(bstr);

// ...

VariantClear(&var);
SysFreeString(bstr); // Wrong, the string has already been freed

VARIANT var1;
VariantInit(&var1);

var1.vt = VT_BSTR;
var1.bstrVal = SysAllocString(L"foo");

// ...

VARIANT var2;
VariantInit(&var2);

var2.vt = VT_BSTR;
var2.bstrVal = var1.bstrVal;
// This is not a copy. This should be considered a transfer of ownership of the string from var1 to var2.
// To do a copy here, use: var2.bstrVal = SysAllocString(var1.bstrVal);

// ...

VariantClear(&var1); // Wrong, var2 now owns the string. var1 should not be cleared.
VariantClear(&var2); // This will free the string twice.

VARIANTARG var;
VariantInit(&var);

var.vt = VT_BSTR;
var.bstrVal = SysAllocString(L"foo");
VariantClear(&var);

// This intends to reuse var.
var.vt = VT_BSTR;
if (/* some condition */) {
    var.bstrVal = SysAllocString(L"bar");
    // ...
}
VariantClear(&var); // Wrong, VariantClear hasn't cleared bstrVal.
// If we don't enter the condition, the string will be freed twice.

Comment 1

[Ray Kraesig [:rkraesig]]

I have a horrible question:

Would it be feasible to intercept SysFreeString to keep a cache of, say, the 4 most-recently-freed addresses, and then crash early with a specific annotation naming the offending module if it double-frees a string? (And then also intercept SysAllocString to remove same-address reallocations from that cache, of course.)

Comment 2

[Yannis Juglaret [:yannis]]

If the goal is to split the volume on this crash signature then I think we could already do that by adding a new rule for signature generation, without requiring interception?