Why Microsoft CRT is so permissive regarding a BSTR double free

Question

This is a simplified question for the one I asked here. I'm using VS2010 (CRT v100) and it doesn't complain, in any way ever, when i double free a BSTR.

BSTR s1=SysAllocString(L"test");
SysFreeString(s1);
SysFreeString(s1);

You expect the *compiler* to have intimate knowledge of the semantics of *system calls*? — Damien_The_Unbeliever, Aug 28 '15 at 08:02
I would have asked the same question about the _System_. So why Windows CRT is so permissive on that? — mbelaoucha, Aug 28 '15 at 08:08
SysFreeString is a function like any other. The compiler doesn't know about it specifically. Welcome to C/C++ programming, were you, as a programmer, need to understand many things to make it work. If you're uncomfortable with this environment, there are "managed" world were everything (almost, or at least, more) is handled for you: java, .net, scripting languages, etc. If you want to move to C++, then there are "smart" things (pointer, classes, etc.), like CComBSTR or _bstr_t: http://stackoverflow.com/questions/2288834/ccomvariant-vs-variant-t-ccombstr-vs-bstr-t — Simon Mourier, Aug 28 '15 at 08:50
If you see no complains it doesn't matter nothing bad happens. — sharptooth, Aug 28 '15 at 09:47

score 2 · Answer 1 · answered Aug 28 '15 at 12:41

Ok, the question is highly hypothetical (actually, the answer is :).

SysFreeString takes a BSTR, which is a pointer, which actually is a number which has a specific semantic. This means that you can provide any value as an argument to the function, not just a valid BSTR or a BSTR which was valid moments ago. In order for SysFreeString to recognize invalid values, it would need to know all the valid BSTRs and to check against all of them. You can imagine the price of that.

Besides, it is consistent with other C, C++, COM or Windows APIs: free, delete, CloseHandle, IUnknown::Release... all of them expect YOU to know whether the argument is eligible for releasing.

score 1 · Answer 2 · answered Aug 28 '15 at 12:52

1

In a nutshell your question is: "I am calling SysFreeString with an invalid argument. Why compiler allows me this".

Visual C++ compiler allows the call and does not issue a warning because the call itself is valid: there is a match of argument type, the API function is good, this can be converted to binary code that executes. The compiler has no knowledge whether your argument is valid or not, you are responsible to track this yourselves.

The API function on the other hand expects that you pass valid argument. It might or might not check its validity. Documentation says about the argument: "The previously allocated string". So the value is okay for the first call, but afterward the pointer value is no longer a valid argument for the second call and behavior is basically undefined.

answered Aug 28 '15 at 12:52

Roman R.

68,205
6
94
158

I agree about the undefined behavior of the second free. And the attention programmers should have regarding these kind of issues. I just wonder why Microsoft is so permissive about this serious issue. Just replace `SysAllocString, SysFreeString` by `malloc, free` or `new, delete` in the same code and see the result. By the way, The documentation does not state any thing about undefined behavior. The BSTR i pass to the 2nd SysFreeString had been allocation by a SysAllocString. – mbelaoucha Aug 28 '15 at 14:34
1

Even though you are technically correct that the argument is still obtained from `SysAllocString` and is "okay" for multiple `SysFreeString` calls because documentation does not explicitly say that once the memory is deallocated, the pointer is no longer a valid argument, certain things go without saying: having a mention of these scenarios would add close to trivial statements on every MSDN article. With `new`, `delete` the compiler throws in some helpful checks to detect errors, but `Sys*` set of functions goes as regular API, there is no context specific assistance with it. – Roman R. Aug 28 '15 at 14:44

Hans Passant · Answer 3 · 2015-08-28T13:16:19.763

1

Nothing to do with the CRT, this is a winapi function. Which is C based, a language that has always given programmers enough lengths of rope to hang themselves by invoking UB with the slightest mistake. Fast and easy-to-port has forever been at odds with safe and secure.

SysFreeString() doesn't win any prizes, clearly it should have had a BOOL return type. But it can't, the IMalloc::Free() interface function was fumbled a long time ago. Nothing you can't fix yourself:

BOOL SafeSysFreeString(BSTR* str) {
    if (str == NULL) {
        SetLastError(ERROR_INVALID_ARGUMENT);
        return FALSE;
    }
    SysFreeString(*str);
    *str = NULL;
    return TRUE;
}

Don't hesitate to yell louder, RaiseException() gives a pretty good bang that is hard to ignore. But writing COM code in C is cruel and unusual punishment, outlawed by the Geneva Convention on Programmers Rights. Use the _bstr_t or CComBSTR C++ wrapper types instead.

But do watch out when you slice the BSTR out of them, they can't help when you don't or can't use them consistently. Which is how you got into trouble with that VARIANT. Always pay extra attention when you have to leave the safety of the wrapper, there are C sharks out there.

edited Aug 28 '15 at 13:16

answered Aug 28 '15 at 13:10

Hans Passant

922,412
146
1,693
2,536

I wanted to know why Microsoft is permissive about such an issue. The C/C++ standards define clearly the undefined behavior of such pratice. Standard C/C++ implementations of `new`, `delete`, `[x]alloc`, and `free` immediately raise an issue for such program. – mbelaoucha Aug 28 '15 at 14:37
By the way, the code is a simpler version of a trickier code. Where the double free was caused by a duplication of a CComVariant. Errors may be more subtle, that is why a double-free should be exposed immediately. – mbelaoucha Aug 28 '15 at 14:40
1

It isn't very clear to me what you expect Microsoft to do about it. They can't stop you from making that function call. If they raise an exception then programs run slower because of the required added checking and 10% of all business-critical software falls over and stops working. Not a great way to get people to upgrade their Windows version, Vista rubbed that in pretty well. – Hans Passant Aug 28 '15 at 14:46
I like your "10% of all business-critical software falls over and stops working". Do you think it is all about ensuring backward-compatibility for buggy software? – mbelaoucha Aug 28 '15 at 14:59
1

I like this phrase: "The C/C++ standards define clearly the undefined behavior". Well, what makes you think that calling SysFreeString twice with the same pointer is not "undefined behavior" as well? There are tons of examples where doing something that is supposed to be "undefined behavior" does not produce any errors or exceptions under certain conditions – yms Aug 28 '15 at 17:04
I agree. The undefined behavior do not necessary lead to an error. But it is 'nice-to-have' an implementation of an undefined behavior to raise an immediate error. Such permissivity is too dangerous in my opinion especially when allowed to beginners (as I with COM programming). – mbelaoucha Aug 31 '15 at 12:12
Moreover, what I pointed is that the documentation does not clearly state any undefined behavior related to `SysFreeString`. So I do not know what to think about double freeing a BSTR. – mbelaoucha Aug 31 '15 at 12:15

yms · Answer 4 · 2015-08-28T17:19:20.907

1

See this quote from MSDN:

Automation may cache the space allocated for BSTRs. This speeds up the SysAllocString/SysFreeString sequence.

(...)if the application allocates a BSTR and frees it, the free block of memory is put into the BSTR cache by Automation(...)

This may explain why calling SysFreeString(...) twice with the same pointer does not produce a crash,since the memory is still available (kind of).

edited Aug 28 '15 at 17:19

answered Aug 28 '15 at 17:02

yms

10,361
3
38
68

thanks a lot for your contribution. I do confirm that the BSTR is somehow cached after. I still see its internal content even after dozen `SysFreeString` calls. But my question wasn't about **HOW** but **WHY** this is permissible. The suggestion of "Hans PASSANT" (in comments) on backward compatibility is more accurate. I would like to have your opinion on that suggestion – mbelaoucha Aug 31 '15 at 12:06
I think the most *authorized* explanation is in the quote I added from MSDN: **This speeds up the SysAllocString/SysFreeString sequence.** As Microsoft said, they don't actually deallocate/reallocate memory for BSTRs when they can use the cache. This is probably some kind of [memory pool](https://en.wikipedia.org/wiki/Memory_pool), which is a mechanism commonly used in scenarios that require lots of memory allocations/deallocations. – yms Aug 31 '15 at 13:28

Why Microsoft CRT is so permissive regarding a BSTR double free

4 Answers4