truncating any reads greater than 4k

[Katana-Steel]

the read_buffer and write_buffer are only designed to be 4k big reading more than 4k to them messes with byte ordering in the MPQ file, tileset\Jungle.vr4 (entry 1809) block header has entries at 5006bytes (the 75th block).

if length_read ever gets larger than 4k, truncating it to 4k

this is a dirty hack... but extraction completes.

[Katana-Steel]

Please tell me why I'm wrong... and what happens when length_read ever exceeds 0x1000 (4k)

what happens to me is in PNG creation of the tileset, free(image); throws a SIGABT with invalid size.

[Hypexed]

This is another issue that should be fixed. Having glanced over the code I wonder, is it worth refactoring it with defines so that the buffer can be easily increased? Or, should it already account for buffer overflow? And should it already divide the work buffer length into blocks of 4KB that it works with so no overflow will occur?

What hardware and OS are you running it on? I just did a test, by patching the code to check for overflow, and on that tileset\Jungle.vr4 file I got nothing! Using Linux Mint 18 x86-x64 on old laptop with Core2 Duo and 2GB Ram myself.

This was my patch inserted like this:

        for (j = 0; j < (num_block - 1); j++) {
            length_read = *(file_header + j + 1) - *(file_header + j);      // . get length of block to read
if (length_read>0x1000) printf("WARNING: length_read=%ld\n",length_read);
            if (fread(read_buffer, sizeof(char), length_read, fpMpq) == 0)      // . read block
                return 1;
            if (flag & 0x30000) {
                Decode((UInt32 *) read_buffer, massive_base, crc_file, length_read / 4);        // . decode block (if file is coded)
            }
            if (length_read == 0x1000 || (j == num_block - 2 && length_read == (size_unpack & 0xFFF))) {        // . if block is unpacked (its length=0x1000 or its last block and length=remainder)
if (length_read>0x1000) printf("WARNING: length_read=%ld\n",length_read);
                memcpy(mpqptr, read_buffer, length_read);
                mpqptr += length_read;
//              fwrite(read_buffer, sizeof(char), length_read, fp_new);     // . write block "as is"
            }

[timfel]

Not sure what to do with this, @Hypexed and @Katana-Steel, if you can agree on what the patch should look like, I'm happy to merge whatever you two propose.

[Hypexed]

I'm not sure either. As much as I hate it when developers want a repeatable test case they can confirm them selves, as well as a problem description and crash logs, because they are such scientists; I think this is what we need here. I ran it myself on old, but more or less common x64 hardware, and nothing turned up.

However, Katana did say it happened in PNG creation during a free() routine. So that would be a good place to check. Apart from that we also to know what kind of MPQ it came from. This could be the cause, a slightly different main MPQ file, causing it to corrupt.

Without this information I don't think anything should be done until Katana gets back with more. One main problem with truncating the length is that the length can also be 4KB or 384KB. So just cutting it back won't help there.

In the overflow the following would be true:

(j == num_block - 2 && length_read == (size_unpack & 0xFFF))

But to me, even if it compiles correctly, it's too vague and needs more parenthesis.

Just checked out the PNG saver. Don't have much experience with PNG API but it does look slightly messy. Needing a setjmp() also looks messy and that section is unfixed as it doesn't free all data, as per the comment. Apart from that it opens file, then sets up PNG handle and then mallocs line array memory. But, if all goes well, it does not free in opposite order, to dispose it all cleanly. Good idea to rearrange that.

[Katana-Steel]

I think I should mention, I'm using the Gold edition CD version of StarCraft I've added some logging as suggested by @Hypexed and pushed it here: https://github.com/Katana-Steel/stargus/commit/6de12206df36d0bfd7b0722fc7e08cb3e2522076

extracted: tileset\Jungle.wpe (1811, 1024 bytes)
Warning file: tileset\Jungle.vr4
read size:5004 bytes at block: 74 metod match: '0x08'
extracted: tileset\Jungle.vr4 (1809, 1524352 bytes)
extracted: tileset\Jungle.vx4 (1810, 161472 bytes)
extracted: tileset\Jungle.cv5 (1806, 82056 bytes)
extracted: tileset\Jungle.vf4 (1808, 161472 bytes)

the PNG free breaks I think because the associated buffers used to merge each of the tileset files overlap or in some other way messing up the stack, caused by the overflow that I'm getting.

the over-sized read looks to be caused by CMpq::explode setting length_write larger than 0x1000... and possibly stack trashing...

[Katana-Steel]

with this https://github.com/Katana-Steel/stargus/commit/ff4ffc3243ad6d320ab1152358a9308792999fda i created this file: file_list.txt

[Hypexed]

I'm currently testing it with a Starcraft Archive from a Mac CD. And getting a segfault on font12. But, this is from my big endian build so it may be a bug in code. The font converter crashes when converting font to image. That also comes from explode routine. Maybe similar issue.

Solution. Replace explode routine?

[Hypexed]

I would have seen this before if I knew better what the routine was doing. But I noticed what you said about the read and write buffer only being 4KB and examined the code. The fact is it is designed to only process 4096 bytes at a time. So for compressed data, the max coming in per file block is 4096 or less and max going out always 4096. Which would explain why cutting it back, even if it looks like a dirty hack, would work. The work buffer is right after the write buffer so an overwrite will corrupt the exploder work buffer. And that could also have a nasty crash.

At this point I also saw a problem with the read_data() routine. It always copies back as many bytes as wanted without checking how many are there. The params struct is also a problem here as it only holds buffers and no buffer sizes. I think this needs refactoring to only give as many bytes as available. The explode() routine always wants 2048 bytes but if the coded data block is smaller that's not a good idea. It looks like it checks to see if data is written over 4096 bytes, but only for one pass.

I'm currently working on rewriting that read routine to only give as much as it has. Though I still think the memory copy into another buffer looks redundant. Will upload some code on a branch when ready.

[Katana-Steel]

Thank you @Hypexed, I have no idea what the explode function does... and when I looked at it, it looked like a complicated mess TBH :) just saw that it set length_write > 4k through GDB, and even when I allocated 8k for both read and write buffers and 32k for explode buffer and expanded the entire buffer with the extra allocations up front. It would still crash startool

[Hypexed]

That explode function unpacks PKWare DCL compressed data. And you are right. It is a mess. It looks to be sourced from this post below which describes it as reverse engineered. From Diablo on Mac OS PowerPC. This makes sense as when I first saw it and even now I thought it was some machine code or an ASM disassembly that was put through an ASM to C converter. What doesn't make sense is that the code is incompatible with PowerPC. It's possible the code used little endian instructions. Or was modified to be optimsed for little endian. But putting that effort into either seems unlikely. https://groups.google.com/forum/#!topic/comp.compression/5Pl0VXN97o4

That's a good effort you did to track it down. There's supposed to be a terminating code in the data so even a buffer overflow shouldn't occur. But even after expanding the buffers it still broke. Given it sends the result downstream which can be converted to another format means it can still crash there as well. Which is what I experienced when the font converter had a corrupt data stream.

In the meantime this is my current fix. I don't know if it fixes the issue. But it only gives the explode routine as much data as there is. In mpq.cpp I modified the params struct like this and that read_data() routine as follows:

typedef struct {
    UInt8 *buf_in;
    UInt8 *buf_out;
    UInt16 size_in;
} params;

UInt16 read_data(UInt8 *buffer, UInt16 size, void *crap)
{
    params *param;

    param = (params *)crap;
    if (size > param->size_in) size = param->size_in;
    if (size) {
        memcpy(buffer, param->buf_in, size);
        param->buf_in += size;
        param->size_in -= size;
    }
    return size;
}

[timfel]

closing in favor of complete refactoring