Bug 13358 - Samba fails DELETE_ON_CLOSE open but leaves newly created file
Samba fails DELETE_ON_CLOSE open but leaves newly created file
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.6.7
All Linux
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-27 18:23 UTC by Steve French
Modified: 2018-04-10 07:31 UTC (History)
5 users (show)

See Also:


Attachments
wireshark-trace-to-samba (2.46 KB, application/x-pcapng)
2018-03-27 18:33 UTC, Steve French
no flags Details
delete_on_close create to Windows 2016 (1.83 KB, application/x-pcapng)
2018-03-27 18:58 UTC, Steve French
no flags Details
Here's the naive fix as a patch. (800 bytes, patch)
2018-03-27 19:22 UTC, Jeremy Allison
no flags Details
Here's the naive fix as a patch. (798 bytes, patch)
2018-03-27 19:24 UTC, Jeremy Allison
sfrench: review+
Details
git-am fix for master. (15.37 KB, patch)
2018-03-28 21:25 UTC, Jeremy Allison
sfrench: review+
Details
git-am fix for 4.8.next. (16.29 KB, patch)
2018-03-30 20:25 UTC, Jeremy Allison
vl: review+
Details
git-am fix for 4.7.next. (16.29 KB, patch)
2018-03-30 20:25 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steve French 2018-03-27 18:23:48 UTC
Samba server (4.6.7-Ubuntu, but also seems to be a problem with more recent as well) when an SMB3 create call is sent with DELETE_ON_CLOSE but only GENERIC_READ| GENERIC_WRITE (not including delete permission requested on the open) - Samba creates the file, but does not clean it up after the open fails with NT_STATUS_ACCESS_DENIED.

See attached trace.

This obviously is a leak which could cause problems if client repeated this with random file names.
Comment 1 Jeremy Allison 2018-03-27 18:28:54 UTC
What does Windows do in the same case ? Does it fail the create, or implicitly add DELETE_ACCESS under the covers if DELETE_ON_CLOSE is sent on create ?

Can you get a wireshark trace/info on how Windows server behaves here ?
Comment 2 Steve French 2018-03-27 18:33:03 UTC
Created attachment 14086 [details]
wireshark-trace-to-samba
Comment 3 Volker Lendecke 2018-03-27 18:51:39 UTC
A quick torture test would be very handy... :-)
Comment 4 Steve French 2018-03-27 18:58:24 UTC
Windows behaves as expected (fails, but file does not persist after close).  See attached trace.
Comment 5 Steve French 2018-03-27 18:58:59 UTC
Created attachment 14088 [details]
delete_on_close create to Windows 2016
Comment 6 Jeremy Allison 2018-03-27 19:04:26 UTC
What do you mean by Windows "fails" here ? You mean the create fails ?
Comment 7 Jeremy Allison 2018-03-27 19:05:33 UTC
Ah, OK - never mind. The create fails with ACCESS_DENIED if initial delete on close is set, but no DELETE access requested. We can do that..
Comment 8 Jeremy Allison 2018-03-27 19:22:38 UTC
Created attachment 14089 [details]
Here's the naive fix as a patch.

We need a torture test for this though.
Comment 9 Jeremy Allison 2018-03-27 19:24:11 UTC
Created attachment 14090 [details]
Here's the naive fix as a patch.

Now does the return :-).
Comment 10 Jeremy Allison 2018-03-27 19:28:52 UTC
Hmmm. source4/torture/basic/delete.c:deltest9() should already be testing this. Wonder how this passes...
Comment 11 Jeremy Allison 2018-03-27 19:50:25 UTC
Ah - deltest9 checks the create fails, but doesn't check if the file exists afterwards.
Comment 12 Jeremy Allison 2018-03-27 23:25:33 UTC
FYI, I have a patch for this - just re-working the test suites to check the things it needs to check.
Comment 13 Jeremy Allison 2018-03-28 20:22:03 UTC
Hang on a minute Steve, I'm about to send a full fix including regression tests for files and directories to the list :-).
Comment 14 Jeremy Allison 2018-03-28 21:25:40 UTC
Created attachment 14092 [details]
git-am fix for master.

Full fix, including regression tests, for master.
Comment 15 Steve French 2018-03-29 01:36:26 UTC
Comment on attachment 14092 [details]
git-am fix for master.

I verified that this fixes the bug I noticed (I had been experimenting with O_TMPFILE patch ideas for cifs vfs smb3 mounts when I noticed this when I left off DELETE on the requested access)
Comment 16 Steve French 2018-03-29 01:46:37 UTC
Comment on attachment 14092 [details]
git-am fix for master.

I did get one error in an affected test running the new torture tests against current Samba server 

failure: deltest20 [
../source4/torture/basic/delete.c:1898: Expression `fnum1 == -1' failed: smbcli_open succeeded, should have failed with NT_STATUS_DELETE_PENDING
]

(there was a similar error on two other tests so this may be an existing problem
test: deltest16a
time: 2018-03-29 01:44:24.575701
WARNING!: ../source4/torture/basic/delete.c:66: Expression `expect_it == io.all_info.out.delete_pending' failed: ../source4/torture/basic/delete.c:1060 - Expected del_on_close flag 0, qfileinfo/all_info gave 1
WARNING!: ../source4/torture/basic/delete.c:97: status was NT_STATUS_DELETE_PENDING, expected NT_STATUS_OK: qpathinfo failed (../source4/torture/basic/delete.c:1061)
time: 2018-03-29 01:44:24.597493
failure: deltest16a [
../source4/torture/basic/delete.c:1068: Expression `fnum1 != -1' failed: open of \delete.file failed (NT_STATUS_OBJECT_NAME_NOT_FOUND)
]
test: deltest17
time: 2018-03-29 01:44:24.597512
time: 2018-03-29 01:44:24.617708
success: deltest17
test: deltest17a
time: 2018-03-29 01:44:24.617723
time: 2018-03-29 01:44:24.637357
failure: deltest17a [
../source4/torture/basic/delete.c:1239: Expression `fnum1 != -1' failed: open - 3 of \delete.file failed (NT_STATUS_OBJECT_NAME_NOT_FOUND)
]
Comment 17 Steve French 2018-03-29 01:47:46 UTC
/usr/local/samba/sbin/smbd -V
Version 4.9.0pre1-GIT-2aeff6f7137 with jra's patch series
Comment 18 Jeremy Allison 2018-03-29 15:41:35 UTC
That test is already a knownfail (has problems against some versions of Windows too). I'll add the back-ports from master once it goes in.
Comment 19 Jeremy Allison 2018-03-30 20:25:15 UTC
Created attachment 14096 [details]
git-am fix for 4.8.next.

Cherry-pick from master.
Comment 20 Jeremy Allison 2018-03-30 20:25:54 UTC
Created attachment 14097 [details]
git-am fix for 4.7.next.

Back-ported from master.
Comment 21 Karolin Seeger 2018-04-04 10:21:02 UTC
Pushed to autobuild-v4-[7,8]-test.
Comment 22 Karolin Seeger 2018-04-10 07:31:30 UTC
(In reply to Karolin Seeger from comment #21)
Pushed to both branches.
Closing out bug report.

Thanks!