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

[Stefan Metzmacher]

Hi Swen,

> 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 !?

You're right, it's confusing, but in this case the memory it not
stolen, it's lifetime is just increased.

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

You're typically right, in this particular case I don't think
it's a real bug, while I agree it's confusing legacy.

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

The correct way to do it is like
smbd_smb2_request_allocate() and schedule_aio_smb2_write().
In smbd_smb2_request_allocate() we first allocate on talloc_tos(),
(which is based on talloc_stackframe_pool(8192) from
smbd_tevent_trace_callback()), so we avoid calling malloc(),
but then we immediately reparent the memory to the longterm xconn
memory context and schedule_aio_smb2_write() doesn't have to
do any talloc_move/steal/reparent magic.

If you want to go the full distance and you need to directly
reparent the talloc_tos() memory directly in construct_reply()
and audit all SMB1 code to correctly free the memory again,
by making sure we only ever call srv_send_smb() from within
smb_request_done(). And we need to prove that we have tests
for all the changed code pathes, but adding smb_panic(__location__)
and find the correct magic to trigger it:
TDB_NO_FSYNC=1 make -j test FAIL_IMMEDIATELY=1
SOCKET_WRAPPER_KEEP_PCAP=1 TESTS="magic"

It would be nice to get there, but I'm not it's worth the effort.
Maybe Volker, Jeremy or Ralph have some additional thoughts about that.

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/20180207/33d170a4/signature.sig>