Larry Osterman's WebLog

Yes, it's time for another "What's wrong with this code"...

Today's example of bad coding involves an attempt to speed up file copy operations by overlapping the read and write operations to the file.  The theory is that the system can be reading from one part of the source file while its writing to another part of the destination.

To do this, the author of the code (ok, it was me, constructing a contrived example - this isn't The Daily WTF) tried (quite badly it turns out) to perform the file I/O by queueing the work to worker threads.

struct FileCopyBlock
{
    LARGE_INTEGER fileDataOffset;
    DWORD readLength;
    HANDLE sourceHandle;
    HANDLE destinationHandle;
    PLARGE_INTEGER dataLengthWritten;
};
const DWORD readChunkSize = 0x10000;

DWORD WINAPI CopyFileDataChunk(LPVOID Context)
{
    FileCopyBlock *dataBlock = (FileCopyBlock *)Context;
    BYTE *dataBuffer = new BYTE[dataBlock->readLength];
    if (dataBuffer == NULL)
    {
        printf("Unable to allocate data block for copy\n");
        exit(1);
    }
    if (!SetFilePointerEx(dataBlock->sourceHandle, dataBlock->fileDataOffset, NULL, FILE_BEGIN))
    {
        printf("Unable to set current file position in source to %i64d: %d", dataBlock->fileDataOffset, GetLastError());
        exit(1);
    }
    DWORD readLength;
    if (!ReadFile(dataBlock->sourceHandle, dataBuffer, dataBlock->readLength, &readLength, NULL))
    {
        printf("Unable to read data from file: %d\n", GetLastError());
        exit(1);
    }
    if (readLength != dataBlock->readLength)
    {
        printf("Unable to read all from file: %d\n", GetLastError());
        exit(1);
    }
    if (!SetFilePointerEx(dataBlock->destinationHandle, dataBlock->fileDataOffset, NULL, FILE_BEGIN))
    {
        printf("Unable to set current file position in destination to %i64d: %d\n", dataBlock->fileDataOffset, GetLastError());
        exit(1);
    }
    DWORD writeLength;
    if (!WriteFile(dataBlock->destinationHandle, dataBuffer, dataBlock->readLength, &writeLength, NULL))
    {
        printf("Unable to read data from file: %d\n", GetLastError());
        exit(1);
    }
    if (writeLength != dataBlock->readLength)
    {
        printf("Unable to write all from file - out of disk space?\n");
        exit(1);
    }
    //
    // Account for data read.
    //
    dataBlock->dataLengthWritten->QuadPart += dataBlock->readLength;
    //
    // Free our buffers
    //
    delete [] dataBuffer;
    delete dataBlock;
    return 0;
}

void MyCopyFile(LPCTSTR SourceFile, LPCTSTR DestinationFile)
{
    HANDLE sourceHandle;
    HANDLE destinationHandle;

    sourceHandle = CreateFile(SourceFile, FILE_READ_DATA, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if (sourceHandle == INVALID_HANDLE_VALUE)
    {
        printf("Error opening source: %d\n", GetLastError());
        return;
    }
    destinationHandle = CreateFile(DestinationFile, FILE_WRITE_DATA, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, sourceHandle);
    if (destinationHandle == INVALID_HANDLE_VALUE)
    {
        DWORD error = GetLastError();
        CloseHandle(sourceHandle);
        printf("Error opening destination: %d\n", error);
        return;
    }
    LARGE_INTEGER fileSize;
    if (!GetFileSizeEx(sourceHandle, &fileSize))
    {
        printf("Unable to get file size: %d\n", GetLastError());
        CloseHandle(sourceHandle);
        CloseHandle(destinationHandle);
        return;
    }
    //
    // Break the read up to 64K chunks, queuing it to a worker thread to do the copy.
    //
    LARGE_INTEGER currentOffset;
    LARGE_INTEGER fileSizeRemaining;
    LARGE_INTEGER fileSizeCopied;
    fileSizeRemaining.QuadPart = fileSize.QuadPart;
    fileSizeCopied.QuadPart = 0;
    currentOffset.QuadPart = 0;
    while (fileSizeRemaining.QuadPart != 0)
    {
        FileCopyBlock *copyBlock = new FileCopyBlock;
        if (copyBlock == NULL)
        {   
            printf("Unable to allocate copy block\n");
            CloseHandle(sourceHandle);
            CloseHandle(destinationHandle);
            return;
        }
        if (fileSizeRemaining.QuadPart < readChunkSize)
        {
            copyBlock->readLength = fileSizeRemaining.LowPart;
        }
        else
        {
            copyBlock->readLength = readChunkSize;
        }
        copyBlock->fileDataOffset = currentOffset;
        copyBlock->sourceHandle = sourceHandle;
        copyBlock->destinationHandle = destinationHandle;
        copyBlock->dataLengthWritten = &fileSizeCopied;
        currentOffset.QuadPart += copyBlock->readLength;
        fileSizeRemaining.QuadPart -= copyBlock->readLength;

        QueueUserWorkItem(CopyFileDataChunk, copyBlock, WT_EXECUTEINIOTHREAD);
    }

    while (fileSizeCopied.QuadPart != fileSize.QuadPart)
    {
        Sleep(100);
    }
    CloseHandle(sourceHandle);
    CloseHandle(destinationHandle);
}

In addition, the code to handle the end of read (sleeping 100) is indescribably cheesy - it's horribly inefficient and defeats the primary purpose of making the I/O overlapped - if you time this file copy routine, it will always take a multiple of 100ms because of the sleep - which wastes a lot of time on small files.

The code also doesn't handle ACLs and alternate data streams (although it should handle file attributes and EAs).  This is also not a bug.  And the file always creates the destination even if it exists, again, that's by design.

But even with all these caveats, there are still at least five major bugs in this code.  Three of them are serious design flaws, one is a complicated implementation issue, and the other one of them is a simple implementation issue.

The fascinating thing about this is that even with these flaws, the code works just fine in many cases - I was unable to introduce the failure using my test harness, even though the errors are quite fundamental.

As always, tomorrow will be the answers and kudos for those who found the problem (and those who found problems I missed while testing this).