Destructors for local function parameters

B7568a7d781a2ebebe3fa176215ae667
0
Wernaeh 101 Aug 09, 2009 at 05:21

Hello everyone :)

Due to upgrading a certain program to VS .Net 2005, parts of the existing codebase stopped working - no crashes, but some windows API calls now fail.

The problem has been reduced to code as the following:

String retrieveObjectProperty();
/*...*/
SomeWindowsCall( ..., retrieveObjectProperty().getInternalString(), ... );

The String object returned by the inner property query method houses its own dynamically allocated storage (dynamic C-style char* array).

Obviously, calling retrieveObjectProperty() creates a copy of the internal string on the stack for return. Now, getInternalString() retrieves the internal character sequence representation of that temporary object and passes it on to the OS.

The problem now, I think, is that the string itself is destroyed before the API call actually is performed, and thus, the internal char* is deleted as well.

Does anybody know what the standard has to say in that matter ?

When are destructors called for temporary objects within expressions of some kind ?

Thank you for your input :)
Cheers,
- Wernaeh

9 Replies

Please log in or register to post a reply.

8fd4a055522ce713cde7dd1cb4083cb2
0
martinsm 101 Aug 09, 2009 at 08:59

The problem now, I think, is that the string itself is destroyed before the API call actually is performed, and thus, the internal char* is deleted as well.

No, that is wrong. Temporary object destructors are executed after SomeWindowsCall function call. This is in C++ standard.

36b416ed76cbaff49c8f6b7511458883
0
poita 101 Aug 09, 2009 at 13:54

Can you be more specific about how the call “fails”? You say it doesn’t crash, so what is that actual observed behaviour?

(And yeah, martinsm is correct about the destructor).

B7568a7d781a2ebebe3fa176215ae667
0
Wernaeh 101 Aug 09, 2009 at 15:26

Thank you for your input!

Okay, that explains why it didn’t give any problems earlier - similar constructs are used throughout the entire codebase, and a small testing program I wrote confirmed the destruction order.

The problem API call is in the following lines, in detail, it’s the buildWindowClassName() call, together with CreateWindow():

    WNDCLASSEX wc =
        { sizeof(WNDCLASSEX),
          CS_OWNDC | CS_HREDRAW | CS_VREDRAW,
          &staticWndProc, 0, sizeof(MicrosoftWindow*), inst,
          IconHandle, CursorHandle, BackdropBrush,
          NULL, buildWindowClassName().getChars(), NULL };

    if (!RegisterClassEx(&wc))
        LOG_AND_THROW_EXCEPTION("Failed to register window class");

    WindowHandle =
        CreateWindow(buildWindowClassName().getChars(),
                     getCaption().getChars(), 0,
                     0, 0, 0, 0,
                     NULL, NULL, inst, NULL);

    if (!WindowHandle)
        LOG_AND_THROW_EXCEPTION("Failed to create window");

The latter exception is thrown, and GetLastError() returns 1407, which is the code for “Cannot find window class”.

If I rewrite the code, as to first build the window class name once, and then save the result in a local variable, using the local variable in both calls, everything works out alright.

I already checked buildWindowClassName(), it always returns a new, identical string.

Perhaps Windows uses the string pointer, rather than the actual string contents, for naming the window class ? Both pointers are obviously different…

The local variable actually is a better solution in that case - but I’d still like to know where the old code went wrong.

Cheers,
- Wernaeh

8fd4a055522ce713cde7dd1cb4083cb2
0
martinsm 101 Aug 09, 2009 at 16:27
WNDCLASSEX wc = { ..., buildWindowClassName().getChars(), ... };
if (!RegisterClassEx(&wc)) ...

This is illegal C++ code (more precisly - it gives undefined behavior).
What happens here - you get temporary string from buildWindowClassName function. And then store const char* pointer to temporary memory in wc struct member (I assume your getChars() method works similarly as c_str() method for std::string). This is OK. But now before next line starts to execute, this temporary memory for string gets freed. So const char* pointer in wc member now points to memory that has been deallocated. RegisterClassEx function now will receive const char* to invalid memory location.

Temporary variables are alive only during the statement they are executed! They are destroyed before next statement starts to execute.

B7568a7d781a2ebebe3fa176215ae667
0
Wernaeh 101 Aug 09, 2009 at 19:32

This is illegal C++ code (more precisly - it gives undefined behavior).
But now before next line starts to execute, this temporary memory for string gets freed.

Godammit, of course - that’s the problem…

Thanks for pointing it out - previously, the window was a singleton instance, and thus only needed just a single window class, which came in as a literal string constant…

Then, some guy (namely, that was me :) ) blindly refractored code around there, and added multiple windows, and replaced the literal constant by the buildWindowClassName() calls without thinking it over…

Now, I’m really ashamed of myself, I’ll go hide under my bed now…

Thank you very much for your help!

Cheers,
- Wernaeh

46407cc1bdfbd2db4f6e8876d74f990a
0
Kenneth_Gorking 101 Aug 10, 2009 at 13:51

@martinsm

This is illegal C++ code (more precisly - it gives undefined behavior).

It’s neither illegal, nor is it undefined. Infact, it does exactly what the standard says it is supposed to do… :blink:

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 Aug 10, 2009 at 13:58

@martinsm

This is illegal C++ code (more precisly - it gives undefined behavior).

Well, technically, it’s not illegal when it merely results in UB ;)
@Kenneth Gorking

It’s neither illegal, nor is it undefined. Infact, it does exactly what the standard says it is supposed to do… :D

The UB part is somewhere in the RegisterClassEx() function dereferencing the by then obsolete pointer :). Of course, the function acts as a black box and it could be implemented in any language, so the C++ rules need not apply.

To the TS: you can bind the temporary to a (const) reference. That way, the temporary gets the lifespan of that reference (according to the standard). This at least saves you the copy that would otherwise be necessary when using a local to store the string.

const MyString & className = buildWindowsClassName();
WNDCLASSEX wc = { ..., className.getChars(), ... };
RegisterClassEx(&wc);
46407cc1bdfbd2db4f6e8876d74f990a
0
Kenneth_Gorking 101 Aug 10, 2009 at 15:19

@.oisyn

The UB part is somewhere in the RegisterClassEx() function dereferencing the by then obsolete pointer :). Of course, the function acts as a black box and it could be implemented in any language, so the C++ rules need not apply.

I was referring to the destructor part, as I think martinsm was aswell, and not the API call (maybe I’m mistaking).

The API call actually doesn’t result in UB: it is getting a rubbish pointer, returns an error, and says it “Cannot find window class”, as one would expect.

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 Aug 11, 2009 at 09:52

No, the CreateWindow() call returns that result, as that function gets the valid class name string, which was never registered as RegisterWindow() was passed a bogus pointer. The key here is the RegisterWindow() function dereferencing the already deleted pointer to register the class name, which is, by definition, UB. It could work, but it also could not. It could even format your harddrive, albeit extremely unlikely.