Bug 13648 - race condition in MoveFile & MoveFileEx -> SMB_COM_RENAME (0x07)
Summary: race condition in MoveFile & MoveFileEx -> SMB_COM_RENAME (0x07)
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-07 19:54 UTC by Joshua Hudson
Modified: 2018-10-07 19:54 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Hudson 2018-10-07 19:54:17 UTC
I located a bug in edge cases when looking for how to deal with a race
condition involving MoveFile targeting set-top NAS boxes. This bug exists in every version of smb that ever existed. OS really should be all *n?x which is not an option.

In particular, process one does:

    handle1 = CreateFile("\\nas\share\drop\working.pid1", ...");
    // fill message file handle1
    if (!CloseHandle(handle1))
        goto io_error; // Out of disk propagating upwards from
reiserfs happens here.
    long long tm = lltime();
    for (int i = 0; i < 1000000000; i++)
    {
        sprintf(newname, "\\nas\share\drop\%lld_%d", tm, i);
        if (!MoveFile("\\nas\share\drop\working.pid2", newname)) {
            if (GetLastError() != ERROR_FILE_EXISTS) {
                goto io_error;
            }
        } else
            return strdup(newname);
    }

while at the same process does the same thing with working.pid.
lltime() is simply a function that works like time() but returning a
long long, thus guaranteeing we don't hit the 2038 bug.

Bug exists in source4/ntvfs/posix/pfs_sys.c:

int pvfs_sys_rename(struct pvfs_state *pvfs, const char *name1, const char *name2, bool allow_override)
{
   //...
   ret = rename(name1, name2);
   //...
}

I think the correct code is as follows but I'm not absolutely certain if allow_override doesn't mean something else:


int pvfs_sys_rename(struct pvfs_state *pvfs, const char *name1, const char *name2, bool allow_override)
{
   //...
#ifdef HAVE_RENAMEAT2
   ret = renameat2(AT_FDCWD, name1, AT_FDCWD, name2, allow_override ? 0 : RENAME_NOREPLACE);
#else
   ret = rename(name1, name2);
#endif
   //...
}

and in source3/modules/vfs_default.c:

static int vfswrap_rename(...)
{
    //...
    result = rename(smb_fname_src->base_name, smb_fname_dst->base_name);
    //...
}

Should read


static int vfswrap_rename(...)
{
    //...
#ifdef HAVE_RENAMEAT2
    result = renameat2(AT_FDCWD, smb_fname_src->base_name, AT_FDCWD,
smb_fname_dst->base_name, RENAME_NOREPLACE);
#else
    result = rename(smb_fname_src->base_name, smb_fname_dst->base_name);
#endif
    //...
}

https://msdn.microsoft.com/en-us/library/ee441650.aspx reads in part:
"If there is an existing file with the new name, the rename MUST fail
with STATUS_OBJECT_NAME_COLLISION." Thus, the race exits in the smb
handling code. Pretty much all linux kernels in the past 10 years or
more have renameat2 so the set-top NAS boxes would be fixed by a
recompile.

I refuse to decide whether degraded functionality is better than no functionality on a kernel that still doesn't have renameat2(). Both linux and BSD kernels have it right now.