devkitPro / newlib

fork from sourceware git://sourceware.org / newlib-cygwin.git
https://devkitpro.org
GNU General Public License v2.0
22 stars 16 forks source link

devkitarm: file read data corruption #16

Closed bgK closed 4 years ago

bgK commented 4 years ago

While investigating this downstream bug, I've noticed an issue with a patch devkitPro does to newlib's fread function: https://bugs.scummvm.org/ticket/11342

The program does the following calls to trigger data corruption:

    char *buffer = (char *)malloc(20*1024);

    // Open a file for reading with buffered IO
    FILE *f = fopen("sdmc:/ScummVM/The Dig/DIG.LA1", "r");

    // Go somewhere in the file, unimportant
    fseek(f, 23800663, SEEK_SET);
    // Initialize the stdio read buffer
    fread(buffer, 1, 1, f);
    // Make a large read that enters devkitPro's fread patched code path.
    // After this, the stdio read buffer is out of sync with the stream position.
    fread(buffer, 1, 10096, f);
    // Seek back a bit in the file. The target seek position is inside the buffer from
    // stdio's perspective (but actually is not as the buffer is out of sync with the actual
    // stream position).
    fseek(f, 23810752, SEEK_SET);
    // Make a small read. The data is fetched from the stdio buffer. It is inconsistent with what is actually in the file.
    fread(buffer, 1, 4, f);

    fclose(f);

I suggest reverting this patch: https://github.com/devkitPro/newlib/blob/a7caafc1971a52eaeb86eed0d59179bcb42e1ddc/newlib/libc/stdio/fread.c#L231

fgsfdsfgs commented 4 years ago

Can confirm I've encountered this bug on Switch, especially when reading large files like PK3 archives in Quake 3.

mtheall commented 4 years ago

I have tested with the fread.c file reverted to original newlib code and this sequence of events still reads the wrong data.

False alarm

WinterMute commented 4 years ago

this is fixed with devkitARM r54 and devkitA64 r15