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

[Swen Schillig]

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, which isn't the case
of course. (That on its own is very bad in my eyes)
In addition this "moved" memory is even further referenced afterwards
via the "old" ptr.

I cannot prove a mem-after-free situation, correct, and it might have
even worked all the time, fine, but if someone "stole" the mem of me,
which is pretty much what's left from the talloc_move, then I
cannot/shouldn't access the memory anymore.
I thought this is the concept of move / steal !?

> 
> > Here's the possible call stack
> > 
> > void reply_write_and_X(struct smb_request *req)
> > {
> > .....
> > 	if(numtowrite == 0) {
> > 		nwritten = 0;
> > 	} else {
> > 		if (req->unread_bytes == 0) {
> > 			status = schedule_aio_write_and_X(conn,
> > 						req,	<==== 
> > 						fsp,
> > 						data,
> > 						startpos,
> > 						numtowrite);
> > 
> > ...
> > out:
> > 	if (req->unread_bytes) {		<=== here we access req
> > which was talloc_moved above
> > 		/* writeX failed. drain socket. */
> > 		if (drain_socket(xconn->transport.sock, req-
> > >unread_bytes) !=
> > 				req->unread_bytes) {
> > 			smb_panic("failed to drain pending bytes");
> > 		}
> > 		req->unread_bytes = 0;
> > 	}
> > }
> 
> talloc_move() is a bit strange, but it's not talloc_free(),
> so the memory is still valid.
I disagree here, the memory might still be there, but I cannot rely on
it and therefore I mustn't access the mem.

> 
> > and within schedule_aio_write_and_X we do what's wrong
> > 
> > NTSTATUS schedule_aio_write_and_X(connection_struct *conn,
> > 			      struct smb_request *smbreq,	<=====
> > 			      files_struct *fsp, const char *data,
> > 			      off_t startpos,
> > 			      size_t numtowrite)
> > {
> > ...
> > aio_ex->smbreq = talloc_move(aio_ex, &smbreq); <=== zero'ing smbreq
> > is 
> > 						    only having local 
> > 						    effect but the 
> > 						    stealing part
> > 						    is problematic
> > ...
> > }
> 
> The same here.
> 
> > Am I missing something ?
> > ...and why is talloc_memdup wrong ?
> 
> talloc_memdup() only copies the struct itself,
> but doesn't take care of talloc children.
You are right !
Crap, I didn't see that one.

> We really want talloc_move/talloc_steal here.
> which moves the smb_request from the short term
> talloc_stackframe() memory to more long term memory
> for async processing.
Ok, if that's what's wanted we need to change the code in the
caller...maybe copy the required fields to local vars.

What do you think ?

Cheers Swen