9 replies to this topic

[Wernaeh]

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

[martinsm]

Quote

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.

[poita]

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).

[Wernaeh]

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

[martinsm]

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.

[Wernaeh]

Quote

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

[Kenneth Gorking]

martinsm said:

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:

[.oisyn]

martinsm said:

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 said:

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

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);

[Kenneth Gorking]

.oisyn said:

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.

[.oisyn]

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.