Re: netapi32: Set the buffer to NULL in NetApiBufferFree

[Juan Lang]

Kai Blin wrote:
> This fixes a Valgrind warning triggered by the NetApiBufferFree() test
> where a buffer is free()d twice.

@@ -52,6 +52,7 @@ NET_API_STATUS WINAPI NetApiBufferFree(LPVOID Buffer)
 {
     TRACE("(%p)\n", Buffer);
     HeapFree(GetProcessHeap(), 0, Buffer);
+    Buffer = NULL;
     return NERR_Success;
 }

I don't get it.  How does setting a local variable to NULL avoid a
warning about freeing it twice?

I'm not saying the patch is wrong, I'm just saying I don't get how
Valgrind is tricked by this.
--Juan

netapi32: Set the buffer to NULL in NetApiBufferFree

[Kai Blin]

On Thursday 26 June 2008 16:34:51 you wrote:
> @@ -52,6 +52,7 @@ NET_API_STATUS WINAPI NetApiBufferFree(LPVOID Buffer)
>  {
>      TRACE("(%p)\n", Buffer);
>      HeapFree(GetProcessHeap(), 0, Buffer);
> +    Buffer = NULL;
>      return NERR_Success;
>  }
>
> I don't get it.  How does setting a local variable to NULL avoid a
> warning about freeing it twice?
>
> I'm not saying the patch is wrong, I'm just saying I don't get how
> Valgrind is tricked by this.

The trick is the test code.
> From dlls/netapi32/tests/apibuf.c, lines 44-58

/* test normal logic */
    ok(pNetApiBufferAllocate(1024, (LPVOID *)&p) == NERR_Success,
       "Reserved memory\n");
    ok(pNetApiBufferSize(p, &dwSize) == NERR_Success, "Got size\n");
    ok(dwSize >= 1024, "The size is correct\n");

    ok(pNetApiBufferReallocate(p, 1500, (LPVOID *) &p) == NERR_Success,
       "Reallocated\n");
    ok(pNetApiBufferSize(p, &dwSize) == NERR_Success, "Got size\n");
    ok(dwSize >= 1500, "The size is correct\n");

    ok(pNetApiBufferFree(p) == NERR_Success, "Freed\n");

    /* test errors handling */
    ok(pNetApiBufferFree(p) == NERR_Success, "Freed\n"); <-- valgrind warning


Which is to be expected. HeapFree on a NULL pointer doesn't hurt, though. As 
this test seems to work on Windows, that seemed like the quickest fix.

Cheers,
Kai

-- 
Kai Blin
WorldForge developer  http://www.worldforge.org/
Wine developer        http://wiki.winehq.org/KaiBlin
Samba team member     http://www.samba.org/samba/team/
--
Will code for cotton.

netapi32: Set the buffer to NULL in NetApiBufferFree

[Reece Dunn]

2008/6/26 Kai Blin <kai.blin(a)gmail.com>:
> On Thursday 26 June 2008 16:34:51 you wrote:
> > @@ -52,6 +52,7 @@ NET_API_STATUS WINAPI NetApiBufferFree(LPVOID Buffer)
> >  {
> >      TRACE("(%p)\n", Buffer);
> >      HeapFree(GetProcessHeap(), 0, Buffer);
> > +    Buffer = NULL;
> >      return NERR_Success;
> >  }
> >
> > I don't get it.  How does setting a local variable to NULL avoid a
> > warning about freeing it twice?
>
> The trick is the test code.
>
> From dlls/netapi32/tests/apibuf.c, lines 44-58
> ...
>
> ok(pNetApiBufferFree(p) == NERR_Success, "Freed\n");
>
> /* test errors handling */
>    ok(pNetApiBufferFree(p) == NERR_Success, "Freed\n"); <-- valgrind warning
>
> Which is to be expected. HeapFree on a NULL pointer doesn't hurt, though. As
> this test seems to work on Windows, that seemed like the quickest fix.

The fix should be
    *Buffer = NULL;
not
    Buffer = NULL;
since p (in the tests) would still be valid after the NetApiBufferFree call.

NOTE: You'll need to add a test for NetApiBufferFree(NULL) if there
isn't one to see if null-pointer dereferencing should be checked
against.

There should be ok(p) or ok(!p) tests to check if this is being set to
NULL on Windows after the call.

Also, GetLastError calls may be needed.

- Reece

netapi32: Set the buffer to NULL in NetApiBufferFree

[Juan Lang]

> The fix should be
>    *Buffer = NULL;
> not
>    Buffer = NULL;
> since p (in the tests) would still be valid after the NetApiBufferFree call.

That's also not correct, as you'd be dereferencing an invalid pointer.
 NetApiBufferFree takes a void *, not a void ** (unlike
NetApiBufferAllocate).

My question remains:  why does setting a local variable to NULL
silence a Valgrind warning?  That sounds like a Valgrind bug to me.
--Juan

netapi32: Set the buffer to NULL in NetApiBufferFree

[John Klehm]

On Thu, Jun 26, 2008 at 10:50 AM, Juan Lang <juan.lang(a)gmail.com> wrote:
> > The fix should be
> >    *Buffer = NULL;
> > not
> >    Buffer = NULL;
> > since p (in the tests) would still be valid after the NetApiBufferFree call.
>
> That's also not correct, as you'd be dereferencing an invalid pointer.
>  NetApiBufferFree takes a void *, not a void ** (unlike
> NetApiBufferAllocate).
>
> My question remains:  why does setting a local variable to NULL
> silence a Valgrind warning?  That sounds like a Valgrind bug to me.

Not that I know anything but it seems that HeapFree calls RtlFreeHeap
and RtlFreeHeap checks to make sure the pointer is not null before
trying to free it.  So explicitly setting it to null makes the erorr
go away.

Now trying to free a pointer a second time could maybe be considered
poor logic but I'll leave that up to wiser minds.

Best,
--John Klehm

netapi32: Set the buffer to NULL in NetApiBufferFree

[Juan Lang]

> Not that I know anything but it seems that HeapFree calls RtlFreeHeap
> and RtlFreeHeap checks to make sure the pointer is not null before
> trying to free it.  So explicitly setting it to null makes the erorr
> go away.

That might be true if setting Buffer to NULL happened before the call
to HeapFree, but it happens after.  After the return from
NetApiBufferFree, Buffer goes out of scope, and a subsequent call
won't be affected by it.

So again, this seems like a Valgrind bug.  How is the warning getting
silenced by this assignment?
--Juan

netapi32: Set the buffer to NULL in NetApiBufferFree

[Kai Blin]

On Thursday 26 June 2008 18:07:59 you wrote:
> > Not that I know anything but it seems that HeapFree calls RtlFreeHeap
> > and RtlFreeHeap checks to make sure the pointer is not null before
> > trying to free it.  So explicitly setting it to null makes the erorr
> > go away.
>
> That might be true if setting Buffer to NULL happened before the call
> to HeapFree, but it happens after.  After the return from
> NetApiBufferFree, Buffer goes out of scope, and a subsequent call
> won't be affected by it.

You said Buffer was a local variable, but it's a parameter passed from the 
test.

So, as far as I can see, the test does

Buffer = HeapAlloc(GetProcessHeap(), NULL, 1024);

HeapReAlloc(GetProcessHeap(),  0, Buffer, 1500);

HeapFree(GetProcessHeap(), 0, Buffer);
HeapFree(GetProcessHeap(), 0, Buffer);

That's a double free, right? Valgrind is complaining about the second one. 
HeapFree() on a NULL buffer doesn't do anything, so it's not causing a 
Valgrind error anymore.

Cheers,
Kai

-- 
Kai Blin
WorldForge developer  http://www.worldforge.org/
Wine developer        http://wiki.winehq.org/KaiBlin
Samba team member     http://www.samba.org/samba/team/
--
Will code for cotton.

netapi32: Set the buffer to NULL in NetApiBufferFree

[Juan Lang]

> You said Buffer was a local variable, but it's a parameter passed from the
> test.
> From the point of view of NetApiBufferFree, Buffer is a parameter,
> which is equivalent to a local variable.  In C, all parameters are
> pass-by-value.  You can simulate pass-by-reference by passing a
> pointer, but it's still pass by (pointer) value.
> So, as far as I can see, the test does
>
> Buffer = HeapAlloc(GetProcessHeap(), NULL, 1024);
>
> HeapReAlloc(GetProcessHeap(),  0, Buffer, 1500);
>
> HeapFree(GetProcessHeap(), 0, Buffer);
> HeapFree(GetProcessHeap(), 0, Buffer);
>
> That's a double free, right? Valgrind is complaining about the second one.

Sort of.  I'll get back to your example in a second.  Valgrind rightly
complains about the second, double free.  I think that's what the test
was aiming to check:  whether a double free resulted in NERR_Success
being returned.  Whether that's a valid thing to check might be
debatable, but it does so, and I think the Valgrind warning is
appropriate.

My question is, why does assigning Buffer to NULL in the context of
NetApiBufferFree result in the warning going away?  Remember, the
first instance Buffer is different from the second.  If we label the
first stack frame with ' and the second with '', what we really have
is:

Buffer = HeapAlloc(GetProcessHeap(), NULL, 1024);

HeapReAlloc(GetProcessHeap(),  0, Buffer, 1500);

Buffer' = Buffer; /* A new copy of Buffer is created in the context of
NetApiBufferFree */
HeapFree(GetProcessHeap(), 0, Buffer');
Buffer'' = Buffer; /* A second copy of Buffer is created in the
context of NetApiBufferFree */
HeapFree(GetProcessHeap(), 0, Buffer'');

So setting Buffer' to NULL has no impact on the HeapFree of Buffer''.
The fact that this makes a Valgrind warning disappear still seems like
a Valgrind bug.  If it were doing static analysis, I'd say it's gotten
confused by aliasing.  Since it's doing dynamic analysis, I have to
ask why that's causing the warning to go away.
--Juan

netapi32: Set the buffer to NULL in NetApiBufferFree

[Kai Blin]

On Thursday 26 June 2008 19:51:41 Juan Lang wrote:
> So setting Buffer' to NULL has no impact on the HeapFree of Buffer''.
> The fact that this makes a Valgrind warning disappear still seems like
> a Valgrind bug.  If it were doing static analysis, I'd say it's gotten
> confused by aliasing.  Since it's doing dynamic analysis, I have to
> ask why that's causing the warning to go away.

I didn't test it. Thinking about this a bit more, you're obviously correct. I 
shouldn't write patches before 8am, I guess.

My fault, never mind. Alexandre suggested just killing the second 
NetApiBufferFree() call, so that's what I'm going to do.

Cheers,
Kai

-- 
Kai Blin
WorldForge developer  http://www.worldforge.org/
Wine developer        http://wiki.winehq.org/KaiBlin
Samba team member     http://www.samba.org/samba/team/
--
Will code for cotton.