[PATCHES] Fix smbd open race and render defer_open test less flakey

[Jeremy Allison]

On Thu, Sep 11, 2014 at 06:04:54PM +0200, Michael Adam wrote:
> On 2014-09-10 at 10:21 -0700, Jeremy Allison wrote:
> > On Wed, Sep 10, 2014 at 01:57:44PM +0200, Michael Adam wrote:
> > > 
> > > The first is due to a race in open_file() which I
> > > fixed in the other attached patch: The file vanished
> > > between check for existence and the acl check.
> > > ==> review/push/comments appreciated.
> > 
> > Great catch Michael
> 
> Gosh! After anoter > 1600 runs, defer_open failed for me again
> and showed that the patch may have been too short-sighted:
> 
> In open_file_ntcreate() we do this:
> 
> 	file_existed = smb_fname->st;
> 	...
> 	open_file(...);
> 	...
> 	if (file_existed &&
> 	    !check_same_dev_ino(&saved_stat, &smb_fname->st))
> 	{
> 		close;
> 		fail with NT_STATUS_ACCESS_DENIED;
> 	}
> 
> So I nicely fixed open_file, but the caller stumbles. ;-)

This is a race that's been there forever I think.

We can make the existence->usage race smaller, but
never eliminate it completely (and in most cases
it really doesn't matter).

The window got widened between smbd's when we changed
the code to not hold the lock for so long, but that
didn't (and couldn't) change the underlying problem.

> What about the attached patch which only fails the open
> if it was not us who did the re-create?

The patch is logically correct and will fix the
bug, but misses one subtlety.

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

Modified fix attached - please review !

Thanks,

	Jeremy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-smbd-open-logic-fix.patch
Type: text/x-diff
Size: 1383 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140911/013b6576/attachment.patch>