Blog: Leak-leaking leaks
Moderator: Moderator Team
Re: Blog: Leak-leaking leaks
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.Z98 wrote:https://www.reactos.org/node/846
Re: Blog: Leak-leaking leaks
Eh... let me explain in more detail.
This is the change withthe fix:
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.
This is the change withthe fix:
Now let's check the description of the first param of CreateStreamOnHGlobal from MSDN:- globalStorage = GlobalAlloc(GMEM_FIXED, 0);
- hResult = CreateStreamOnHGlobal(globalStorage, FALSE, &globalStream);
- if (FAILED(hResult))
+ hResult = CreateStreamOnHGlobal(NULL, FALSE, &globalStream);
+ if (FAILED_UNEXPECTEDLY(hResult))
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.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.
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.
Re: Blog: Leak-leaking leaks
Maybe Windows returns NULL from GlobalAlloc(GMEM_FIXED, 0); call, but ReactOS doesn't? I know, you probably tried it as the first step
Re: Blog: Leak-leaking leaks
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)gigaherz wrote:... 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.
Re: Blog: Leak-leaking leaks
@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)...
"... 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)...
Re: Blog: Leak-leaking leaks
I am ignorant, but your decision seems correct to me. Msgina is a replaceable component.gigaherz wrote:...I decided the (shutdown dialog) code in msgina wasn’t designed to be called externally.
http://technet.microsoft.com/en-us/library/bb742447.aspx wrote: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.
http://msdn.microsoft.com/en-us/library/windows/desktop/aa378750.aspx wrote: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.
-
- Posts: 1790
- Joined: Fri Aug 07, 2009 5:11 am
- Location: USA
Re: Blog: Leak-leaking leaks
Why not run Coverity and similar on the BrowseUI code to help look for leaks?
Re: Blog: Leak-leaking leaks
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 casegigaherz wrote: 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.
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.
Re: Blog: Leak-leaking leaks
[deleted]
Last edited by dsp8195 on Tue Jul 22, 2014 8:51 am, edited 1 time in total.
Re: Blog: Leak-leaking leaks
This looks like a leak to me. Shellbrowser.cpp 1837
HRESULT STDMETHODCALLTYPE CShellBrowser::RemoveMenusSB(HMENU hmenuShared)
{
if (hmenuShared == fCurrentMenuBar)
fCurrentMenuBar = NULL;
return S_OK;
}
HRESULT STDMETHODCALLTYPE CShellBrowser::RemoveMenusSB(HMENU hmenuShared)
{
if (hmenuShared == fCurrentMenuBar)
fCurrentMenuBar = NULL;
return S_OK;
}
Re: Blog: Leak-leaking leaks
I was about to suggest the same. cppcheck could also help.PurpleGurl wrote:Why not run Coverity and similar on the BrowseUI code to help look for leaks?
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 expecteddsp8195 wrote:Great progress so far. Honestly, for the first time ROS got a person who knows what he's doing.
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.Frontier wrote:This looks like a leak to me. Shellbrowser.cpp 1837
HRESULT STDMETHODCALLTYPE CShellBrowser::RemoveMenusSB(HMENU hmenuShared)
{
if (hmenuShared == fCurrentMenuBar)
fCurrentMenuBar = NULL;
return S_OK;
}
Who is online
Users browsing this forum: No registered users and 17 guests