Netchan_CreateFileFragments corrupts fragment header when file size is smaller than header size

[SamVanheer]

When Netchan_CreateFileFragments creates the fragment buffers that will be used to transmit a file it does not correctly account for files that have very small sizes.

Files with a size smaller than strlen(filename) + (bCompressed ? strlen("bz2") : strlen("uncompressed")) + 4 are affected.

The cause of this is in the code that writes the header for the fragment list:

// These variables come from earlier in the function
int filesize = ...;
int chunksize = ...; // Value of cl_dlmax set by client, default 512 bytes
bool bCompressed = ...;

bool isfirstfragment = true;
int bufferid = 1;

while (true)
{
    int fragmentsize = chunksize;

    if ( filesize <= chunksize )
        fragmentsize = filesize;

    fragbuf_t* buf = (fragbuf_t *)Mem_ZeroMalloc(sizeof(fragbuf_t));
    buf->frag_message.data = buf->frag_message_buf;
    buf->bufferid = bufferid++;
    buf->frag_message.cursize = 0;
    buf->frag_message.maxsize = 1400;
    buf->frag_message.buffername = "Frag Buffer Alloc'd";
    buf->next = 0;

    sizebuf_t* sb = &buf->frag_message;
    SZ_Clear(sb);

    if ( isfirstfragment )
    {
        MSG_WriteString(sb, filename);
        if ( bCompressed )
          MSG_WriteString(sb, "bz2");
        else
          MSG_WriteString(sb, "uncompressed");
        MSG_WriteLong(sb, uncompressed_size);
        fragmentsize -= buf->frag_message.cursize;
    }

    filesize -= fragmentsize;
    buf->size = fragmentsize;
    isfirstfragment = false;

    // Rest of loop omitted.
}

The first fragment contains information about the list of fragments: the filename, compression method and the uncompressed size. This is the fragment header.

The size of the header depends on the length of the filename and the compression used. It's subtracted from the fragment size, the remainder is used for the first file fragment chunk.

If the size of the header is larger than the file size then the fragment size becomes a negative value. The filesize will increase resulting in bogus data being written on the receiving end, and more importantly the header will be truncated.

The truncation occurs in Netchan_Transmit:

fragbuf_t* buf = ...;

if ( buf->isfile && buf->isbuffer == false )
{
    if ( buf->iscompressed )
    {
        snprintf(compressedfilename, 0x104u, "%s.ztmp", buf->filename);
        file = FS_Open(compressedfilename, "rb");
    }
    else
    {
        file = FS_Open(buf->filename, "rb");
    }

    FS_Seek(file, buf->foffset, 0);
    FS_Read(&buf->frag_message.data[buf->frag_message.cursize], buf->size, 1, file);
    buf->frag_message.cursize += buf->size;
    FS_Close(file);
}

FS_Read could read a lot of data here (it appears to convert the signed size to unsigned internally without a < 0 check) but since the file is very small it shouldn't be able to overflow the buffer. The line immediately following that is where the contents of the fragment are truncated.

This is best shown using an example.

Given a UTF8-encoded JSON file containing only ASCII characters, total size 2 bytes:

{}

With filename networkdata/crossfire.json the header size will be 44:

• networkdata/crossfire.json is 26 characters plus a null terminator
• The compression method is uncompressed, that's 12 characters plus a null terminator
• 4 bytes for the uncompressed size

This results in the following:

• fragmentsize is 2 in the first iteration
• fragmentsize becomes -42 (2 - 44)
• filesize becomes 44 (2 - -42)
• buf->size is set to -42
• fragmentsize is 44 in the second iteration
• buf->size is set to 44
• When the first fragment is sent the message size is reduced by -42. The initial message size is 44 since the entire header was written, resulting in only 2 bytes being sent
• When the second fragment is sent 44 bytes are sent. Only 2 bytes are read from the file, so the remaining 42 bytes are 0 since the buffer was zero-initialized on allocation

On the receiving side the entire header is read from the first fragment but only the first 2 bytes are valid. The receiver will then read data from subsequent fragments, writing them to an incorrect file and adding the 42 extra 0 bytes.

It just so happens that the resulting filename is the original filename truncated to the first filesize bytes characters. In this case the filename becomes ne. Adding more data increases the length of the name.

The receiver assumes that the data is uncompressed if the compression method is not "bz2" so the uncompressed size is ignored. If the file is compressed and the filename is valid then this will result in garbage data being read.

If the filename is invalid then the receiver will be stuck waiting forever to continue since the received filename does not match any resource in the resourcesneeded list. The game is not frozen since it isn't a blocking operation.

A workaround for text files is to pad the file with whitespace characters until the file size exceeds the largest possible header size. The engine typically stores filenames in buffers of size 64 (MAX_QPATH in Quake 1) so 64 + 13 + 4 = 81 bytes is the minimum size needed to counteract this bug.

To fix this the code that calculates the remaining fragment buffer size for the first fragment needs to check if this is a small file:

if ( isfirstfragment )
{
    MSG_WriteString(sb, filename);
    if ( bCompressed )
      MSG_WriteString(sb, "bz2");
    else
      MSG_WriteString(sb, "uncompressed");
    MSG_WriteLong(sb, uncompressed_size);

    if (fragmentsize < chunksize)
    {
        fragmentsize = min(fragmentsize, chunksize - buf->frag_message.cursize);
    }
    else
    {
        fragmentsize = max(0, fragmentsize - buf->frag_message.cursize);
    }
}

(untested, but should work)

This way the fragment size can't be negative and as much file data as possible is stored in the first fragment.

[di57inct]

Sorry for the offtopic, but can we get this guy a gold medal? Sam you are the savior of goldsrc. Just wanted to pay my full respect to you :)

[a1batross]

This bug was fixed in ReHLDS. See commit as an example of possible engine side fix.