Blog: Leak-leaking leaks

[Z98]

https://www.reactos.org/node/846

[Webunny]

https://www.reactos.org/node/846

It would seem strange Windows would accept a call that was never meant to be there at all, to be frank. Logic would rather indicate it should have *some* use/importance, then.

[gigaherz]

Eh... let me explain in more detail.

This is the change withthe fix:

- globalStorage = GlobalAlloc(GMEM_FIXED, 0);
- hResult = CreateStreamOnHGlobal(globalStorage, FALSE, &globalStream);
- if (FAILED(hResult))
+ hResult = CreateStreamOnHGlobal(NULL, FALSE, &globalStream);
+ if (FAILED_UNEXPECTEDLY(hResult))

Now let's check the description of the first param of CreateStreamOnHGlobal from MSDN:

hGlobal [in]
A memory handle allocated by the GlobalAlloc function, or if NULL a new handle is to be allocated instead. The handle must be allocated as moveable and nondiscardable.

As you can see, the function can work in two ways, either you give it a handle to an already allocated buffer (which may or may not contain valid data), or you let the function allocate its own.

The code chose to allocate a buffer, BUT it was a fixed buffer, AND of size 0. Note here that fixed is the opposite of moveable, not fixed-length. The problem with this is that While windows is happy to work with this buffer, even though it's not the kind it's supposed to accept, ReactOS just errors. Removing the GlobalAlloc and letting the function create its own buffer works correctly in both Windows and ReactOS, so I made the change.

[Black_Fox]

Maybe Windows returns NULL from GlobalAlloc(GMEM_FIXED, 0); call, but ReactOS doesn't? I know, you probably tried it as the first step

[hbelusca]

... The problem with this is that While windows is happy to work with this buffer, even though it's not the kind it's supposed to accept, ReactOS just errors. Removing the GlobalAlloc and letting the function create its own buffer works correctly in both Windows and ReactOS, so I made the change.

You know, there are things called wine- or api-tests. That would be nice to make (if not already existing) tests for this(these) function(s). And (I'm looking at the community): "Patches are welcome" (tm)

[hbelusca]

@gigaherz:
"... I had been told that there was a JIRA issue related to the shutdown dialog, and that it had been implemented in some way in trunk already. I considered the means of calling that dialog from the shell32 function the start menu calls, but I dropped the idea some hours later, when I decided the code in msgina wasn’t designed to be called externally. At some point someone will have to copy the needed code over to shell32, and replicate the dialog there. ..."

(CORE-7559)

It might be that, on windows, shell32 makes calls to ordinal-only APIs from msgina, that seem to be stubbed out in our code, as it seems to do to retrieve the graphics bar logo when displaying the ShellAbout dialog (CORE-7940)...

[middings]

...I decided the (shutdown dialog) code in msgina wasn’t designed to be called externally.

I am ignorant, but your decision seems correct to me. Msgina is a replaceable component.

Msgina.dll, the standard GINA provided by Microsoft and loaded by Winlogon, can be replaced by a GINA that is custom-built by a third party.

If you are writing a GINA to replace the Microsoft standard GINA DLL (MSGina.dll), you may want to provide some or all of the standard GINA functionality.

[PurpleGurl]

Why not run Coverity and similar on the BrowseUI code to help look for leaks?

[serrox]

Now let's check the description of the first param of CreateStreamOnHGlobal from MSDN:

hGlobal [in]
A memory handle allocated by the GlobalAlloc function, or if NULL a new handle is to be allocated instead. The handle must be allocated as moveable and nondiscardable.

I found 2 articles about CreateStreamOnHGlobal in msdn. and here (http://msdn.microsoft.com/en-us/library ... s.85).aspx) is note about GMEM_FIXED case

CreateStreamOnHGlobal will accept a memory handle allocated with GMEM_FIXED, but this usage is not recommended. HGLOBALs allocated with GMEM_FIXED are not really handles and their value can change when they are reallocated. If the memory handle was allocated with GMEM_FIXED and fDeleteOnRelease is FALSE, the caller must call GetHGlobalFromStream to get the correct handle in order to free it.

[dsp8195]

[deleted]

[Frontier]

This looks like a leak to me. Shellbrowser.cpp 1837

HRESULT STDMETHODCALLTYPE CShellBrowser::RemoveMenusSB(HMENU hmenuShared)
{
if (hmenuShared == fCurrentMenuBar)
fCurrentMenuBar = NULL;
return S_OK;
}

[mrugiero]

Why not run Coverity and similar on the BrowseUI code to help look for leaks?

I was about to suggest the same. cppcheck could also help.

Great progress so far. Honestly, for the first time ROS got a person who knows what he's doing.

Actually, ReactOS got to this point because quite a lot of people knew what they were doing. Also, I think someone with weaker knowledge than whoever wrote that code might also be useful to spot this kind of problems, as they are more likely to need to read the documentation every few lines, and that could help notice when something is not used as expected

This looks like a leak to me. Shellbrowser.cpp 1837

HRESULT STDMETHODCALLTYPE CShellBrowser::RemoveMenusSB(HMENU hmenuShared)
{
if (hmenuShared == fCurrentMenuBar)
fCurrentMenuBar = NULL;
return S_OK;
}

I don't think so. It doesn't look like that function is supposed to free hmenuShared, and setting fCurrentMenuBar to NULL if they are equal means hmenuShared still contains a reference to the pointed location in memory. Accodring to the documentation, it's only supposed to enable you to free the resources. It sounds ambiguous, and I actually read it as "I will not clean up the objects, just remove references inside my member variables", and that's what it's doing.