Just because you’re using a smart pointer class doesn’t mean you can abdicate understanding what it does


It's great when you have a tool to make programming easier, but you still must understand what it does or you're just replacing one set of problems with another set of more subtle problems. For example, we discussed earlier the importance of knowing when your destructor runs. Here's another example, courtesy of my colleague Chris Ashton. This was posted as a Suggestion Box entry, but it's pretty much a complete article on its own.

I came across an interesting bug this weekend that I've never seen described anywhere else, I thought it might be good fodder for your blog.

What do you suppose the following code does?

CComBSTR bstr;
bstr = ::SysAllocStringLen(NULL, 100);
  1. Allocates a BSTR 100 characters long.
  2. Leaks memory and, if you're really lucky, opens the door for an insidious memory corruption.

Obviously I'm writing here, so the answer cannot be A. It is, in fact, B.

The key is that CComBSTR is involved here, so operator= is being invoked. And operator=, as you might recall, does a deep copy of the entire string, not just a shallow copy of the BSTR pointer. But how long does operator= think the string is? Well, since BSTR and LPCOLESTR are equivalent (at least as far as the C++ compiler is concerned), the argument to operator= is an LPCOLESTR – so operator= naturally tries to use the wcslen length of the string, not the SysStringLen length. And in this case, since the string is uninitialized, wcslen often returns a much smaller value than SysStringLen would. As a result, the original 100-character string is leaked, and you get back a buffer that can only hold, say, 25 characters.

The code you really want here is:

CComBSTR bstr;
bstr.Attach(::SysAllocStringLen(NULL, 100));

Or:

CComBSTR bstr(100);

I'm still a big fan of smart pointers (surely the hours spent finding this bug would have been spent finding memory leaks caused by other incautious programmers), but this example gives pause – CComBSTR and some OLE calls just don't mix.

All I can add to this story is an exercise: Chris writes, "Since the string is uninitialized, wcslen often returns a much smaller value than SysStringLen would." Can it possibly return a larger value? Is there a potential read overflow here?

Comments (38)
  1. Anonymous says:

    Of course, if the newly allocated string happens to contain no null characters at all.

    [Try again. -Raymond]
  2. nathan_works says:

    If you don’t write the "new" and the "delete", you don’t know what’s happening with the memory you are handling.. For better or worse..

  3. Anonymous says:

    taken from MSDN SysAllocStringLen :

    cch = The number of characters to be copied from pch. A null character is placed afterwards, allocating a total of cch plus one characters.

    so SysStringLen will stop on that null character and won’t return a larger value.

  4. Anonymous says:

    Sorry Wizou, but pch in the example is NULL. pch is never copied; the string is never initialized and no null terminator is appended. I have, in fact, expertly performed several catastrophic buffer overflows in my lifetime using similar logic, much to my chagrin.

  5. Anonymous says:

    wouldn’t

    CComBSTR bstr = ::SysAllocStringLen(NULL, 100);

    magically fix the problem?

    I always hated extra lines.

  6. Anonymous says:

    The format of a BSTR starts with a 4-byte length specifier so if he allocated 100 characters, it would be 200 or 0x00 00 00 00 C8 and since wcslen accepts a null-terminated string…

    And if that doesn’t stop it the documentation says that the data segment may contain embedded nulls so you have a slight chance of stopping beforehand. ( about 1-(2^16-1)^100/2^16^100?)

    And if that still doesn’t stop wcslen, it’s null-terminated.

  7. Anonymous says:

    @dgt No, this would be the same behavior.

    As Raimond explained, the cause is the compiler does not know the argument is a BST (he sees OLECHAR*).

    @Old Coder

    Wizou is correct, SysAllocStringLen initializes the final null terminator, even if input string is NULL and thus the chars before the final null terminator are garbage.

    For _bstr_t there is an extra ctor with copy flag _bstr_t(BSTR s, bool copy), and from the code it uses the BSTR length of s, not wcslen.

    So this would also correct the original problem.

    Just another question is if uninitialized strings are good style ;-)

  8. Anonymous says:

    For some reason, Raymond’s exercise reminds me of the "Microsoft vs Psychic Friends Network" story floating around on the internet.

    "You have a problem? Could it be that it’s your computer that’s your problem? I sense… is it an unintentional buffer overflow due to misuse of smart pointers?"

  9. Anonymous says:

    @myself:

    I meant wcslen, of course … not SysStringLen

    @Wil:

    The pointer to a BSTR points to the first character of the data string, not to the length prefix.

  10. Anonymous says:

    From the docs for SysAllocStringLen():

    "If pch is NULL, a string of the requested length is allocated, but not initialized."

    Sounds like there’s no guarantee that the buffer will be terminated with anything, much less a L’’, which is what’s required by the CComBSTR::operator=( LPOLESTR) operator.

    There you go – instant buffer overflow opportunity.

    However, I’m not sure that this is really a smart pointer problem – this could happen without smart pointer just as easily, I think. If you’re using raw pointers and throwing around uninitialized data structures, you’re still going to get bad results unless you’re really careful.  It seems to me the problem is in the misunderstood/incorrect use of SysAllocStringLen(), not the CComBSTR class.

  11. Garry Trinder says:

    @BrianK

    Try Again.  In dgt’s example, CComBSTR::operator=() is not being used.  

    CComBSTR bstr = ::SysAllocStringLen(NULL, 100);

    is the equivalent of

    CComBSTR bstr = CComBSTR(::SysAllocStringLen(NULL, 100));

    This will lead to the same problem, but via a different route.

  12. Anonymous says:

    I think the point dgt is trying to make is that

    CComBSTR bstr = ::SysAllocStringLen(NULL, 100);

    does NOT call operator=. It’s an initialization statement, and therefore it will use a constructor.  The above line is essentially just another way to say this…

    CComBSTR bstr(::SysAllocStringLen(NULL, 100));

    Whether this works around the problem, I have no idea.

  13. Anonymous says:

    Nasty code you’ve got there.

    I blame the language…and the platform. There has to be a better way to do this stuff that doesn’t require .NET. In the previous example I’m not sure why that CComBSTR class can’t just be replaced by a string class built off of BSTRs. It would handle the icky job of managing the lifetime of the string.

    This, of course, is complicated by other scenarios: when you have to take over the lifetime of other BSTRs, or you want to release your internal BSTR for others, but this is well-trodden territory. Since strings are so common, it makes sense to use a higher level of abstraction when dealing with them so there is less chance of programmer error.

  14. Anonymous says:

    CComBSTR bstr(::SysAllocStringLen(NULL, 100));

    Whether this works around the problem, I

    have no idea.

    No, Of course it doesn’t.

    And you know this because you could write:

    CComBSTR bstr(L"Hello");

    No one is freeing that memory allocated by SysAllocStringLen and passed to the contructor.

  15. Anonymous says:

    That’s c++ for you, it’s impossible by design to avoid all the cases.

    For example, developers invariably try to make functions that return COM object pointers as returned values, instead of parameters

    Example:

    CComPtr<IUnknown> spObj = ::CreateMyObject();

    instead of

    ::CreateMyObject( &spObj);

    Same problem, it leaks.  Mixing C and C++ is deadly, you still have to know how these things works.

    The best way it to not mix the coding style, like it happens with the CComBSTR and ::SysAllocString example.

  16. Anonymous says:

    I once tracked down a similar problem:

    void function(CComBSTR oldGuy)

    {

    CComBSTR newGuy;

    newGuy = oldGuy.copy();

    … }

  17. Anonymous says:

    Chris Ashton? The conlanger that created Vendi? The world is so small.

  18. BryanK says:

    dgt: Absolutely *not* (…I don’t think; C++ has been confusing for a very long time now).

    The whole problem is that you’re assigning the result of ::SysAllocStringLen() to a CComBSTR, and using operator= to do it.  That operator= call resolves to the overload that takes an LPCOLESTR, so the operator= code uses wcslen, which probably returns the wrong value, since the string contains garbage.  (Exactly as the original post said.)

    Doing the assignment in a single line doesn’t make any difference: you’re still calling operator=, and you’re still calling it with an LPCOLESTR — as far as the compiler knows, anyway.

    You *need* to use CComBSTR::Attach to hook a CComBSTR up to a BSTR that you happen to have, otherwise you *will* get the wrong answer.  Or, you can skip the API call entirely, as in the second correct example above, and call the CComBSTR constructor that takes a length.

    Wil: Yes, a BSTR starts with a 4-byte length specifier, but that length specifier is *BEFORE* the address that the BSTR pointer contains.  So if you get a BSTR pointer whose value is 0x12345678, then the length is the four bytes at 0x12345674.  The first character of the string is (the two bytes starting) at 0x12345678.

    So as long as there aren’t any embedded pairs of zero bytes (there can be single zero bytes, because this is a UTF-16 string), wcslen would work fine when passed the BSTR variable’s value.  But as you said, the contents of the string are uninitialized: if there are any pairs of zero bytes in the garbage, wcslen will stop too early.

    The link that Raymond provided above to the definition of a BSTR claims that there’s always a zero terminator after cch characters’ worth of bytes (200 here).  So that will stop wcslen, you’re right there.  Unless it’s off by one, I don’t think wcslen can return more bytes than ::SysAllocStringLen(NULL, x); passed for "x".

  19. Anonymous says:

    While I agree with your claim, the argument justifying it is dubious at best.

    The problem is that the semantics and the syntax of this smart-pointer class are opaque and fragile.  

    While you need to understand smart pointers before you use them, the class designer should give at least a little thought to making the correct usage "simple" and making incorrect usage "hard".  boost’s various smart pointers are a perfectly good example.

    The specific problem in the current example is the assignment operator=().  

    *Unless = can be made work in a completely intuitive fashion that will never trip you up, you should simply not implement it.*  It’s a trap waiting to happen – you’d use and forget "=" (and never notice it while code reviewing) whereas that would never happen if you had a non-const method on your class, SetFromWideString().

    There’s also the other stupidity that BSTR and LPCOLESTR are the same so you never know looking at a function call whether you’re making the terrible mistake of passing a wide char string to a short string routine or vice versa.

    If you were using standard STL, you’d be using two different classes, basic_string<char> and basic_string<wchar>, so there’d be no chance of passing one to the other.

    (I don’t really know these MS things so forgive me if I’m not understanding something obvious because I lack context…)

  20. Anonymous says:

    To tsas yei nahc.  That’s me.  :-)

    Internet is much smaller than world, btw.

  21. Anonymous says:

    @mikeb: Raymond is pointing out [url=http://msdn.microsoft.com/en-us/library/ms221069.aspx]this  MSDN page[/url] which states that a BSTR consists of a length prefix, a data string, and a terminator.

    So I guess we can assume that BSTR-allocating functions always enforce this null terminator. Even for a non-initialized data string.

  22. Anonymous says:

    Matt Green: I blame the language…

    Please don’t blame C++ for this. It’s not C++’s fault that certain vendors decided to build huge APIs on it long before it was standardised and had many of its most useful features developed (Templates, STL, Exceptions, etc). (Yes, I realise that CComBSTR is part of the ATL (oddly named since most of its classes are not actually templates)).

    And despite the headline, CComBSTR isn’t really a smart pointer at all. It’s a wrapper class for a data structure. It’s no more a smart pointer than std::string.

  23. Anonymous says:

    The same kind of problem happens with the Standard C++ Library smart pointer, std::auto_ptr<>.

    If you try to operator= it with a raw pointer, or even =-initialize it with a raw pointer, woe is you: You must explicitly use the constructor and the reset() member function.

  24. Anonymous says:

    @Ulric : I think the CComPtr<> class lacks a non-addref-ing constructor.

    The MFC class COleDispatchDriver allows you to addref or not, and by default its constructor from LPDISPATCH doesn’t.

    This discrepancy between the different smart pointer classes is also a source of problems (I assumed CComPtr<> didn’t AddRef() and had to check its source code to make sure).

  25. Anonymous says:

    A good point well made.

    I believe it was Scott Meyers, in Effective C++ [1], who said "Smart pointers add at least as much complexity as they remove".

    And I think Spolsky’s Law [2] is pertinent too.

    [1] http://www.amazon.com/Effective-Specific-Addison-Wesley-Professional-Computing/dp/0321334876

    [2] http://www.joelonsoftware.com/articles/LeakyAbstractions.html

  26. BryanK says:

    JamesCurran:

    is the equivalent of

    CComBSTR bstr = CComBSTR(::SysAllocStringLen(NULL, 100));

    Ack.  This is (part of the reason) that I hate C++.  In other languages, that would simply call the equivalent of operator=, using the overload that takes an LPOLESTR, since that’s what you wrote.  But no, because this is C++, and because the call happens to be in an initializer (…what?!?!), it goes on some crazy trip through a temp variable whose constructor takes the BSTR (actually probably LPOLESTR?), and then through the  operator= that takes a CComBSTR.  (I think.)

    This is part of what I meant by "C++ has been confusing for a very long time now" — this kind of dependence on (to me, non-obvious) context is hard to keep track of.

    So, OK, dgt: Let me take that back.  Of course, you’ll still (most likely) leak the original BSTR, and the constructor that takes an LP(C)OLESTR will probably get it wrong as well by not using SysStringLen, but that code doesn’t use the operator= that takes an LPCOLESTR.

  27. Anonymous says:

    std::auto_ptr operator= ought to delete the thing that the std::auto_ptr is pointing to before assigning a new raw pointer to it – if it doesn’t do that, I think there is something wrong with your standard library implementation. Now assigning and std::auto_ptr to another std::auto_ptr is where the fun starts. It does have its place though – I use it sometimes for function arguments to indicate that I am taking ownership of the pointer that is being passed in and the caller should not touch it again.

  28. Anonymous says:

    Construction is when you create a variable that had not been there before.

    Assignment is when you change the value of an existing variable.

    Construction always uses constructors, hence the name ;-)

  29. Anonymous says:

    @Wizou: I’m not sure what Raymond is intending by his "try again" link.  The fact is that the docs for SysAllocStringLen() says that if a NULL is passed in, the returned string is not initialized.  If you rely on some undocumented behavior that does initialize the string then, as Raymond has pointed out in previous articles, your code may break when MS changes the implementation to some other behavior that still respects the documented interface.

    In fact, on WinXP SP2 SysAllocStringLen() does place a single null wchar at the end of the allocated buffer (in the case of the SysAllocStringLen( NULL, 100) call, it places the null char just after where the 100th character would go).  But I see that as MS programming defensively – not part of the SysAllocStringLen() interface.

    Also, I’m not sure, but I think that the page Raymond linked to may have an error – it indicates that BSTRs are terminated with two null characters, which would be 4 null bytes (since BSTRs consist of wchars). Is this correct, or do BSTRs normally terminate with a single null wchar?

  30. Anonymous says:

    I still don’t see how the problem Chris Ashton had anything to do with smart pointers (except incidentally).  The same problems would have existed if the result from SysAllocStringLen() had been passed to a normal C function that copied the BSTR into a normal C struct.  The problem was that he was asking some other entity to copy a BSTR that had (at best) only been partially initialized, and was never freeing the buffer returned from the SysAllocStringLen() call.

    And why has auto_ptr<> been brought into this? CComBSTR behaves nothing like auto_ptr<>.

  31. Anonymous says:

    *** correction about my previous message ***

    COleDispatchDriver’s constructor from LPDISPATCH never AddRefs. It merely allows you the option not to release it either, which turns it into a non-smart pointer. Dangerous, I’d say.

  32. Anonymous says:

    @Medinoc

    Since the arrival of boost::smart_ptr, there is no need to use the std::auto_ptr any more. Even the standards-people admit its design being flawed.

    @BryanK

    Javaist ;-)

    But the mnemonic is quite straightforward: "When ist constructs, it uses a constructor. When it assigns, it uses assignment."

    There is a c’tor accepting a BSTR (rather its equivalent LPCOLESTR), and the Compiler uses that instead of first initializing an empty variable using the default c’tor and subsequently overwriting it.

    Constructing a variable will *never* use a member function. Whose member, in the first place?

  33. BryanK says:

    Actually, no: Pythonist.  A pox on the kingdom of nouns!  ;-P

    As far as the mnemonic: That’s great (apart from being recursive), but it doesn’t actually help me much.  How am I supposed to know what "constructs" and what "assigns" in the first place?  It looks like the line that dgt posted is doing an assignment, to me, but apparently it’s doing a construction?  Or something?

    > Whose member, in the first place?

    I generally think of the constructor as a member function.

    Unless you’re asking which object’s operator= call?  I was thinking it’d be an object that was created using the no-argument constructor (of course, I’m assuming that one of those exists for CComBSTR).  Yes, now you’re creating multiple instances and copying stuff around, so maybe that’s not what happens.

    (Maybe it treats it like a single constructor call, so you only ever get a single instance?  That seems to match your middle paragraph’s description.  But that’s even *less* obvious from the code, I think.  Oh well; obviously I don’t know enough of the intricacies of C++.  Which is sort of my point.  ;-) )

  34. Anonymous says:

    As Raymond indicated, the BSTR will be null terminated (the null terminator is part of the data type, as opposed to a char*).

    http://msdn.microsoft.com/en-us/library/ms221069.aspx does say that on an Apple Mac the data string consists of a single byte string. On the Mac (pre os X?) platform, would wcslen still interpret this string as a wide character string? If that were to happen, we would probably have a read overflow unless the string happens to contain a continuous sequence of two null bytes starting at even offsets (0, 2, 4) or a sequence of three continuous nulls.

  35. Weeble says:

    Did somebody give a correct answer? I find the documentation Raymond linked to with his "Try Again" unclear: it says that if a NULL argument is passed, the returned string is "allocated, but not initialized". One would *assume* that this means the length prefix and terminator are still set, just that the data part is uninitialized, because it would be a bit useless otherwise, but making assumptions doesn’t seem a particularly good idea.

    If it does indeed set the terminator, then I can’t see a predictable way for wcslen to return a higher value than SysStringLen would, although I think the behaviour is undefined if there’s insufficient memory for SysAllocStringLen, resulting in a NULL argument to wcslen, but I don’t think that’s what Raymond’s looking for.

  36. Anonymous says:

    Let’s assume the string returned by SysAllocString happens to have a 0 whcar_t before the end of the buffer. Then bstr will actually get a buffer that is less than 100 characters in length. Now, when subsequent code uses the buffer (assuming 100 characters of space are available), memory corruption can happen.

  37. Anonymous says:

    obviously the problem comes down to 1 thing: type safety.

    the moment that class decided a BSTR was something else (a LPCTSTR), things started to go wrong.

    the moral of this is: always enforce strong typing. Do not use #define to ‘reuse’ another type, create a class and give it a few conversion operators.

    CComBSTR b = (LPCTSTR)mybstr; is at the very least obvious that you’ve taken responsibility for the leak, if there were 2 operator= for LPCTSTR and BSTR, then this issue would never arise.

    The other thing you could do is remember that CComBSTR is not a smart pointer class, it is a helper for BSTRs like CString is a helper for c-strings. It is not equivalent to CComPtr.

  38. Anonymous says:

    >> the moment that class decided a BSTR was something else (a LPCTSTR), things started to go wrong. <<

    The class didn’t decide the BSTR was an LPCTSTR. Someone at Microsoft decided a BSTR was an OLECHAR*.

    // from afxwin.h:

    typedef OLECHAR* BSTR;

    What would you define it as?

Comments are closed.