Bug 12844 - Related requests with TreeConnect fail with NETWORK_NAME_DELETED
Summary: Related requests with TreeConnect fail with NETWORK_NAME_DELETED
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.6.5
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: 13796
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-15 10:30 UTC by Moritz Bechler
Modified: 2020-08-19 00:26 UTC (History)
2 users (show)

See Also:


Attachments
Packet capture TreeConnect+IoCtl (4.84 KB, application/x-pcapng)
2017-06-15 10:30 UTC, Moritz Bechler
no flags Details
Possible patch for master (848 bytes, patch)
2017-06-15 21:10 UTC, Stefan Metzmacher
no flags Details
git-am fix for 4.6.next, 4.5.next. (960 bytes, patch)
2017-06-19 18:54 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Moritz Bechler 2017-06-15 10:30:27 UTC
Created attachment 13283 [details]
Packet capture TreeConnect+IoCtl

Looks to me that using the TID from the previous tree connect is just not implemented.

/usr/sbin/smbd: smbd_smb2_request_done_ex: idx[1] status[NT_STATUS_OK]
body[16] dyn[no:0] at ../source3/smbd/smb2_tcon.c:170
/usr/sbin/smbd: smbd_smb2_request_dispatch: opcode[SMB2_OP_IOCTL] mid = 13
/usr/sbin/smbd: smbd_smb2_request_error_ex: smbd_smb2_request_error_ex:
idx[5] status[NT_STATUS_NETWORK_NAME_DELETED] || at
../source3/smbd/smb2_server.c:2449
/usr/sbin/smbd: smbd_smb2_request_done_ex: idx[5]
status[NT_STATUS_NETWORK_NAME_DELETED] body[8] dyn[yes:1] at
../source3/smbd/smb2_server.c:3145


Error comes from smbd_smb2_request_check_tcon

1799			in_tid = req->last_tid;
(gdb) print req->last_tid
$1 = 4294967295 (== 0xFFFFFFFF)

which is the unspecified TID from the TREE_CONNECT request.

[SMB2] "3.2.4.1.4 Sending Compounded Requests" would suggest this is legal (and it is handled as expected by MS servers):

3.The client MUST construct the subsequent request as it would do normally. For any subsequent
requests the client MUST set SMB2_FLAGS_RELATED_OPERATIONS in the Flags field of the SMB2
header to indicate that it is using the SessionId, TreeId, and FileId supplied in the previous
request (or generated by the server in processing that request). The client SHOULD<93> set
SessionId to 0xFFFFFFFFFFFFFFFF and TreeId to 0xFFFFFFFF, and SHOULD<94> set FileId to {
0xFFFFFFFFFFFFFFFF, 0xFFFFFFFFFFFFFFFF }.


For reproduction I could offer some test cases using the java implementation if you like.
Comment 1 Stefan Metzmacher 2017-06-15 21:10:31 UTC
Created attachment 13286 [details]
Possible patch for master
Comment 2 Stefan Metzmacher 2017-06-16 09:43:10 UTC
(In reply to Stefan Metzmacher from comment #1)

Does that patch (for tcon) fixes the tcon problem for you?
Comment 3 Moritz Bechler 2017-06-16 10:08:09 UTC
Yes, looks good.
Comment 4 Jeremy Allison 2017-06-16 16:59:57 UTC
(In reply to Stefan Metzmacher from comment #2)

Patch LGTM. RB+. Only requested change is can you move the hunk to the end of that function:

382 

Here please..

+	req->last_tid = tcon->global->tcon_wire_id;
+

383         *out_tree_id = tcon->global->tcon_wire_id;
384         return NT_STATUS_OK;

So it makes it really clear this is a part of setting the outputs on success condition.

I know there are no returns in between where you did put it and the end of function, but putting it there makes is much clearer this is where we're returning the tid on success.

Cheers,

Jeremy.
Comment 5 Stefan Metzmacher 2017-06-16 20:18:07 UTC
(In reply to Jeremy Allison from comment #4)

I added it after the tcon->status = NT_STATUS_OK;
and successful smbXsrv_tcon_update(), before starting to
calculating all out_* values.

But I don't really mind if you move it to the absolute end of the
function after the:

*out_tree_id = tcon->global->tcon_wire_id;

Then please to the same for the session setup patch of bug #12845.

It would also be good to make a test for the tcon change,
but I don't have the time to add that currently.
Comment 6 Jeremy Allison 2017-06-16 20:33:08 UTC
OK, will do (make the changes for the tcon and sessionsetup patches).

Once they're in master I'll take a look at what it would take to add tests, but I think this'll be hard as we don't currently have infrastructure for compounding in the generic libsmbclient code. Might be possible to do it in the source4 cli code.
Comment 7 Jeremy Allison 2017-06-19 18:54:11 UTC
Created attachment 13293 [details]
git-am fix for 4.6.next, 4.5.next.

Cherry-picked from master.
Comment 8 Jeremy Allison 2017-06-20 18:54:00 UTC
Karolin please add to 4.6.next, 4.5.next.
Comment 9 Karolin Seeger 2017-06-23 11:12:54 UTC
(In reply to Jeremy Allison from comment #8)
Pushed to autobuild-v4-{6,5}-test.
Comment 10 Karolin Seeger 2017-06-27 10:46:36 UTC
(In reply to Karolin Seeger from comment #9)
Pushed to both branches.
Closing out bug report.

Thanks!
Comment 11 Stefan Metzmacher 2019-04-15 13:58:02 UTC
The patches on bug #13796 are related...