Race condition between SetFilePointerEx and ReadFile / WriteFile in the mirror sample

[grandemk]

Environment

• Windows version: 10
• Processor architecture: x64
• Dokany version: 2.0.6.1000
• Library type (Dokany/FUSE): Dokany

Check List

• [ x ] I checked my issue doesn't exist yet
• [ x ] My issue is valid with mirror default sample and not specific to my user-mode driver implementation
• [ x ] I can always reproduce the issue with the provided description below.
• [ x ] I have updated Dokany to the latest version and have reboot my computer after.
• [ x ] I tested one of the last snapshot from appveyor CI

Description

Hi :)

There is a race condition in the ReadFile and in the WriteFile function.

If 2 MirrorReadFile happen at the same time, the following run gives a wrong result: MirrorReadFile 2 calls SetFilePointerEx MirrorReadFile 1 calls SetFilePointerEx MirrorReadFile 2 calls ReadFile and uses the offset of MirrorReadFile 1 MirrorReadFile 1 calls ReadFile and uses the offset set after the text that was read by MirrorReadFile 2.

Here is a program to reproduce the issue:

# in one shell
# Create tests/Assets and tests/AssetsVirtual and put Test1.txt in tests/Assets.
# Then mount tests/Assets as tests/AssetsVirtual
$ ./x64/Release/mirror.exe /r "tests/Assets" /l "tests/AssetsVirtual" /e /d /s

# create_test_overlap.py
As='A' * 0x40000
Bs='B' * 0x40000
Cs='C' * 0x40000

s=As + Bs + Cs
print("{}".format(s))

# In another shell
$ python3 create_test_overlap.py > tests/Assets/TestOverlap.txt
$ ./test_overlapping_io.exe
REQ [0]
REQ [3]
REQ [2]
REQ [1]
REQ [0]
REQ [3]
REQ [2]
REQ [1]
[!] Didn't get the same result ! ACC virt: 17039383, ACC real: 51904535

Example of logs when the problem appears
[Debug] GetFileInfo: success, file size = 786434 (tests\Assets\Test\TestOverlap.txt)
[Debug] ReadFile: Byte to read: 262144, Byte read 2, offset 524288 (tests\Assets\Test\TestOverlap.txt)
[Debug] ReadFile: 'tests\Assets\Test\TestOverlap.txt'
[Debug] ReadFile: Byte to read: 262144, Byte read 0, offset 786432 (tests\Assets\Test\TestOverlap.txt)
[Debug] ReadFile: Byte to read: 262144, Byte read 262144, offset 262144 (tests\Assets\Test\TestOverlap.txt)
[Debug] ReadFile: Byte to read: 262144, Byte read 262144, offset 0 (tests\Assets\Test\TestOverlap.txt)

// test_overlapping_io.cpp
#include <Windows.h>
#include <fileapi.h>
#include <stdio.h>

#include <vector>
#include <cassert>

struct TResult
{
    long long int acc = 0;
};

TResult test_overlapped_io(const wchar_t* filename)
{
    TResult res {};

    HANDLE file = CreateFileW(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
    assert(file != INVALID_HANDLE_VALUE);

    int reqNb = 4;
    int bufSize = 0x40000;
    std::vector<std::vector<BYTE>> buffers(reqNb);
    for(auto& buf : buffers)
    {
        buf.resize(bufSize);
        std::fill(buf.begin(), buf.end(), 0);
    }

    std::vector<HANDLE> events(reqNb);
    std::vector<OVERLAPPED> requests(reqNb);
    for(int i = 0; i < reqNb; ++i)
    {
        auto& req = requests[i];
        req.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
        assert(req.hEvent);
        events[i] = req.hEvent;
        req.Offset = i * bufSize;
    }

    for(int i = 0; i < reqNb; ++i)
    {
        if(!ReadFile(file, buffers[i].data(), bufSize, NULL, &requests[i]))
        {
            assert(GetLastError() == ERROR_IO_PENDING);
        }
    }

    // sync
    int numRead = reqNb;
    std::vector<HANDLE> objects(reqNb);
    std::vector<int> indices(reqNb);
    for(int i = 0; i < reqNb; ++i)
    {
        objects[i] = events[i];
        indices[i] = i;
    }

    do
    {
        DWORD waitRes = WaitForMultipleObjects(numRead, objects.data(), FALSE, INFINITE);
        assert(waitRes != WAIT_FAILED);
        if ((waitRes >= WAIT_OBJECT_0) && (waitRes < WAIT_OBJECT_0 + reqNb))
        {
            int objIdx = waitRes - WAIT_OBJECT_0;
            int reqIdx = indices[objIdx];
            printf("REQ [%d]\n", reqIdx);
            for(int i = 0; i < requests[reqIdx].InternalHigh; ++i)
            {
                TCHAR val = (TCHAR)buffers[reqIdx][i];
                //printf("%c", val);
                res.acc += val;
            }
            --numRead;
            if(numRead == 0)
                break;
            objects[objIdx] = objects[numRead];
            indices[objIdx] = indices[numRead];
        }
    } while (true);

    for(int i = 0; i < reqNb; ++i)
    {
        CloseHandle(events[i]);
    }
    CloseHandle(file);

    return res;
}

int main()
{
    const wchar_t* virtual_filename = L"tests/AssetsVirtual/TestOverlap.txt";
    const wchar_t* real_filename = L"tests/Assets/TestOverlap.txt";
    TResult virt = test_overlapped_io(virtual_filename);
    TResult real = test_overlapped_io(real_filename);
    if (virt.acc != real.acc)
        printf("[!] Didn't get the same result ! ACC virt: %lld, ACC real: %lld\n", virt.acc, real.acc);
    return 0;
}

[Liryna]

Hi @grandemk ,

You are correct. There should be a lock wrapping the SetFilePointerEx and ReadFile.

[grandemk]

I think using OVERLAPPED instead of SetFilePointerEx is sufficient to solve the issue Something like this:

     OVERLAPPED overlap;
     memset(&overlap, 0, sizeof(OVERLAPPED));
     overlap.Offset = Offset & 0xFFFFFFFF;
     overlap.OffsetHigh = (Offset >> 32) & 0xFFFFFFFF;
     if (!ReadFile(handle, Buffer, BufferLength, ReadLength, &overlap))

[Liryna]

That can work too! Do you mind creating a pull request with the change ? Would be very appreciated

[Liryna]

Done with https://github.com/dokan-dev/dokany/commit/f225a0d60a1dc3d3656ccef3c6c99401ac66a20b

Thanks @grandemk for the report and providing the solution!