Oh Boy, There’s a Bug

Can you spot it?

void p(const CString & str)
{
char* endPtr = NULL; 
unsigned long x = strtoul(str.Mid(1, 2), 10, &endPtr);
if (*endPtr != ‘\0’) {
// invaild input…
}

}

 Using ATL’s CString, you can run this code and nothing will happen – the bug won’t be ‘triggered’. And more than that, everything will work as expected and correctly. But as most developers find bugs, running the software and fixing the crashes, this one didn’t come up to the surface… so you have a bug and you don’t know about it, how not fun. If you are really into C++ business it might be easy for you to spot this bug. But I’m not sure if it’s C++ to blame here, guess it’s not. If you take a look at the Mid method its declaration is as follows:

CStringT Mid(int iFirst, int nCount) const;

 This means it returns a object with the relevant slice of the original string. The destructor of this new object is immediately called, because its scope is a ‘parameter’. So now the endPtr points to freed memory. Yey. And then you access endPtr as you should…even if the memory is freed it is still in memory (because CString dynamically allocates space for its buffers), only AppVerifier forced it to be removed, thus generating an access-violation and then it surfaced up and we fixed it. But, honestly, it was still tricky to spot this one.

 So now that you know the bug and why it happened, I can only say that you should beware of methods/functions that return objects and has a very short scope, if at all. Not mentioning the copy constructor issues… Of course, merely the scope isn’t to be blamed, it’s the mixture of short scope and a stale pointer (and dynamically allocated memory),…To be honest, if the memory was local to the stack there was no way to detect this bug, unless the compiler would use the same stack space for different variables and they were written to between the call to strtoul and the access to the pointer. And even that isn’t reliable, but you could notice some weird mismatches between the input and that input validation…

 Any suggestions for tracking such bugs?

5 Responses to “Oh Boy, There’s a Bug”

  1. mo says:

    what is the return value of mid ? const char * ?

  2. mo says:

    hi,

    your description of the problem isn’t accurate.
    you say “The destructor of this new object is immediately called, because its scope is a ‘parameter’”. this isn’t accurate.
    the destructor of the temporary object is called at the end of the line (when ‘;’ is reached.

    check this out too:
    http://groups.google.com/group/comp.lang.c++.moderated/msg/00b5030017bed63f

  3. arkon says:

    Yep, you’re right, and that what I actually meant, but I wanted to emphasize the problem. Anyways, it leads to the same consequence that the temporary object won’t exist after the call to strtoul, but rather stay in memory as garbage.

  4. Chris says:

    Also derefencing endPtr which may still be NULL:
    if (*endPtr != ‘′) {

    Could become:
    if ((endPtr != NULL) && (*endPtr != ‘′)) {

  5. arkon says:

    Nope, check out documentation of this function.

Leave a Reply