Bug 10809 - smbd: race condition in file creation
Summary: smbd: race condition in file creation
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.1.12
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-10 20:07 UTC by Michael Adam
Modified: 2014-09-29 18:03 UTC (History)
2 users (show)

See Also:


Attachments
Patch for v4-1-test, cherry-picked from master. (3.65 KB, patch)
2014-09-11 15:18 UTC, Michael Adam
obnox: review+
jra: review+
Details
git-am fix for master. (1.35 KB, patch)
2014-09-11 17:08 UTC, Jeremy Allison
no flags Details
Full set of patches (3 of them) that went into master. (5.06 KB, patch)
2014-09-12 23:19 UTC, Jeremy Allison
no flags Details
updated patchset for 4.1 with cherry-pick- and bug-info (5.32 KB, patch)
2014-09-19 22:37 UTC, Michael Adam
obnox: review+
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Adam 2014-09-10 20:07:40 UTC
there is a race condition in source3/smbd/open.c:open_file().
between checking for the existence of a file and checking
it's acl in open_file(), the file can vanish. In this case
the NT_STATUS_OBJECT_NAME_NOT_FOUND from the acl check
(which is only performed if the file existed) is treated as
an error. Instead the parent access should be checked in this
case and the file should be created if O_CREAT was specified
(i.e. if the disposition was OPEN_IF).

This can be reproduced with the defer_open test on a busy
system, but is only one of several failure causes, and not
the most frequent one.
Comment 1 Michael Adam 2014-09-11 15:18:48 UTC
Created attachment 10281 [details]
Patch for v4-1-test, cherry-picked from master.

Patch for v4-1-test, cherry-picked from master.
Comment 2 Jeremy Allison 2014-09-11 16:04:01 UTC
Comment on attachment 10281 [details]
Patch for v4-1-test, cherry-picked from master.

LGTM.
Comment 3 Jeremy Allison 2014-09-11 16:04:19 UTC
Re-assigning to Karolin for inclusion in 4.1.next.
Comment 4 Michael Adam 2014-09-11 16:06:29 UTC
Gosh, there is a subsequent failure...
The patch does not seem to be complete. :-(
(see samba-technical)

Taking the bug away from Karolin for now.
Comment 5 Jeremy Allison 2014-09-11 16:16:09 UTC
Bah - didn't see that. I'll take a look asap :-).
Jeremy.
Comment 6 Jeremy Allison 2014-09-11 17:08:53 UTC
Created attachment 10282 [details]
git-am fix for master.

This is the additional fix we need I think. From the
mailing list:

As we atomically create using O_CREAT|O_EXCL,
then if new_file_created is true, then
file_existed *MUST* have been false (even
if the file was previously detected as being
there.

We use the variable file_existed again in logic
below this statement, so the correct fix is to
set file_existed = false, if new_file_created
is returned as true from open_file().
Comment 7 Jeremy Allison 2014-09-12 23:19:55 UTC
Created attachment 10286 [details]
Full set of patches (3 of them) that went into master.

Applies cleanly to 4.1.next.
Comment 8 Michael Adam 2014-09-19 19:39:28 UTC
(In reply to comment #7)
> Created attachment 10286 [details]
> Full set of patches (3 of them) that went into master.
> 
> Applies cleanly to 4.1.next.

ACK in principle, but we should re-do the patches with
cherry-pick -x and BUG:.... info.
Comment 9 Michael Adam 2014-09-19 22:37:29 UTC
Created attachment 10298 [details]
updated patchset for 4.1 with cherry-pick- and bug-info

Updated Patch for 4.1.
We should do the cherry-picks with
"git cherry-pick -x ..." so that we see the
git hash from master in the release branch commit.

I also added "BUG: <url>" tags to the two patches
that don't carry them in master.

Michael
Comment 10 Michael Adam 2014-09-19 22:38:35 UTC
Comment on attachment 10298 [details]
updated patchset for 4.1 with cherry-pick- and bug-info

Jeremy, please (re-)ACK this updated patchset.
Only commit messages differ. Clean cherry-picks from master.
Comment 11 Jeremy Allison 2014-09-19 23:04:25 UTC
Comment on attachment 10298 [details]
updated patchset for 4.1 with cherry-pick- and bug-info

LGTM.
Comment 12 Jeremy Allison 2014-09-19 23:04:45 UTC
Re-assigning to Karolin for inclusion in 4.1.next.
Comment 13 Karolin Seeger 2014-09-27 18:02:53 UTC
Pushed to autobuild-v4-1-test.
Comment 14 Karolin Seeger 2014-09-29 18:03:00 UTC
Pushed to v4-1-test.
Closing out big report.

Thanks!