[PATCH] Don't modify memory reference provided by value

Swen Schillig swen at vnet.ibm.com
Wed Feb 21 14:09:41 UTC 2018


On Wed, 2018-02-21 at 14:24 +0100, Andreas Schneider via samba-
technical wrote:
> On Wednesday, 7 February 2018 08:48:52 CET Swen Schillig via samba-
> technical 
> wrote:
> > Hi Metze
> > 
> > Thanks for your continuous efforts, it's really appreciated.
> > 
> > On Tue, 2018-02-06 at 22:34 +0100, Stefan Metzmacher wrote:
> > > Hi Swen,
> > > 
> > > > > what problem are you trying to solve here?
> > > > > 
> > > > > talloc_memdup() is at least wrong here! (The same applies to
> > > > > the
> > > > > push_blocking_lock_request() patch).
> > > > 
> > > > Sorry, I tried to explain it as good as I can.
> > > > 
> > > > The problem is, that memory is accessed after the ownership
> > > > moved.
> > > > ...and even a use-after-free situation could occur.
> > > 
> > > Why is moving the ownership a problem?
> > > We move the request from the short term talloc_stackframe
> > > to more long term memory to enable async processing.
> > > 
> > > Where do you see a use-after-free?
> > > 
> > > I agree this code is not nice, but I don't see a bug.
> > 
> > A few days ago I was made to use talloc_move instead of
> > talloc_steal
> > on a local variable which was going out of scope 2 lines down.
> > 
> > And here it is OK to talloc_move a parameter received by value,
> > implying that it is zero'ed and not used anymore
> 
> talloc_move changes the parent of the memory context and sets the
> pointer 
> argument to NULL. It is not zeroing (memset(0)) the memory!

I know, sorry if I was un-clear in my statements.
But here it is not even doing what's assumed.
The "pointer argument" is provided by value, therefore, only the local
"value" will be set to NULL (what I called zero'ed in my earlier mail)
and not the variable handed over from the caller.

Anyway, I guess it's said about it.
I just surrender.

Thanks to all for taking the time and commenting.

Cheers Swen.




More information about the samba-technical mailing list