Bug 1900964 Comment 0 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Original comment by
on 
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).

One reason for these bugs is that `VariantClear` on a `VT_BSTR` will free the string and some people seem to 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:

```c++
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
```

```c++
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.
```

```c++
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.
```
Revision 1 by
on 
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).

One reason for these bugs is that `VariantClear` on a `VT_BSTR` will free the string and some people seem to 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:

```c++
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
```

```c++
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.
```

```c++
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.
```
Revision 2 by
on 
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 some people seem to 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:

```c++
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
```

```c++
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.
```

```c++
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.
```
Revision 3 by
on 
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:

```c++
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
```

```c++
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.
```

```c++
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.
```

Back to Bug 1900964 Comment 0