Re: advapi32: Implement GetSecurityInfo. [take 3]

[Juan Lang]

Hi Dan,

+    if (ppSecurityDescriptor)
+        *ppSecurityDescriptor = sd;
+

you leak sd in the (implicit) else block here.
--Juan

advapi32: Implement GetSecurityInfo. [take 3]

[Dan Hipschman]

On Thu, Aug 07, 2008 at 02:25:00PM -0700, Juan Lang wrote:
> +    if (ppSecurityDescriptor)
> +        *ppSecurityDescriptor = sd;
> +
>
> you leak sd in the (implicit) else block here.

So does Windows.

advapi32: Implement GetSecurityInfo. [take 3]

[Juan Lang]

> > you leak sd in the (implicit) else block here.
>
> So does Windows.

In that case, you should include that in your tests, as it's not
especially obvious.
--Juan

advapi32: Implement GetSecurityInfo. [take 3]

[Dan Hipschman]

On Thu, Aug 07, 2008 at 02:28:10PM -0700, Dan Hipschman wrote:
> On Thu, Aug 07, 2008 at 02:25:00PM -0700, Juan Lang wrote:
> > +    if (ppSecurityDescriptor)
> > +        *ppSecurityDescriptor = sd;
> > +
> >
> > you leak sd in the (implicit) else block here.
>
> So does Windows.

I guess I should elaborate on that.  You can request any of the four
parts of the security descriptor and pass NULL for the SD itself and
Windows will return them without anyway to free the memory.

advapi32: Implement GetSecurityInfo. [take 3]

[Juan Lang]

> I guess I should elaborate on that.  You can request any of the four
> parts of the security descriptor and pass NULL for the SD itself and
> Windows will return them without anyway to free the memory.

I can see what MSDN says it does, but your tests don't check that
calling GetSecurityInfo with a non-null ppsidOwner, ppsidGroup etc.
and a null ppSecurityDescriptor is allowed.  Because you're not
checking it, it looks like a bug.
--Juan

advapi32: Implement GetSecurityInfo. [take 3]

[Dan Hipschman]

On Thu, Aug 07, 2008 at 02:33:35PM -0700, Juan Lang wrote:
> > I guess I should elaborate on that.  You can request any of the four
> > parts of the security descriptor and pass NULL for the SD itself and
> > Windows will return them without anyway to free the memory.
>
> I can see what MSDN says it does, but your tests don't check that
> calling GetSecurityInfo with a non-null ppsidOwner, ppsidGroup etc.
> and a null ppSecurityDescriptor is allowed.  Because you're not
> checking it, it looks like a bug.

These results were determined through tests that I ran on Windows.  I
don't think I included them with the conformance tests because I didn't
think tests that leaked memory would be very good tests.  I will send
them in a separate patch since the implementation is still valid.

advapi32: Implement GetSecurityInfo. [take 3]

[Vitaliy Margolen]

Juan Lang wrote:
> Hi Dan,
>
> +    if (ppSecurityDescriptor)
> +        *ppSecurityDescriptor = sd;
> +
>
> you leak sd in the (implicit) else block here.
> --Juan

There are no other options here. I'm sure windows will leak it as well. Did 
anyone mentioned already how well win32api is designed?

You can't free it since other pointers refer to it. Unless of course all the 
pointers are NULL. Only then descriptor can be freed.

Vitaliy.