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

[Stefan Metzmacher]

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.

> 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.

> 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.
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.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180206/33916456/signature.sig>