| Summary: | RWops doesn't check for integer overflow when stdio_fseek only supports 32 bits | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Simon Hug <chli.hug> |
| Component: | file | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | sezeroz |
| Version: | HG 2.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
Patch that adds range checks to stdio_seek for 32-bit limited offsets.
seektest 0.1. Fuzzing test program for the seeking API of RWops. Patch that fixes a warning about a comparison with signed and unsigned type. Patch that adds range checks to stdio_seek for 32-bit limited offsets. |
||
Created attachment 2864 [details]
seektest 0.1. Fuzzing test program for the seeking API of RWops.
This is a program which tests the RWops seeking API by throwing some random offsets at it and checking if it reached the right position.
Note that the heavy random IO of this program can cause weird issues on Windows with terabyte-sized sparse files. It seems to reserve virtual memory extremely aggressive and then runs out of it.
Examples
Setup a 5 GiB file with random data. The random data is created with the position as a seed.
seektest -G -o ./bigfile
Test the previous file by seeking to random positions and then comparing the data with the expected random value for that position.
seektest -g -r ./bigfile
Create a file and write values at random positions within 1 TB. Then read the data at the same positions and check if we get the same values back.
seektest -g -w -r -p -o -s 1000000000000 ./hugefile
See all options with -h.
Created attachment 2865 [details]
Patch that fixes a warning about a comparison with signed and unsigned type.
This isn't really related to this issue, but if someone already commits something to SDL_rwops.c this could be included.
There's a line in SDL_rwops.c that compares size_t > Sint64. The attached patch adds a Sint64 cast for the first value.
Created attachment 2918 [details]
Patch that adds range checks to stdio_seek for 32-bit limited offsets.
Updated patch for current tip.
Also added HAVE_LIMITS_H to the other config headers. They all define STDC_HEADERS so they should have it.
Patch tweaked and added, thanks! https://hg.libsdl.org/SDL/rev/e7a79b236dc0 > Patch tweaked and added, thanks!
> https://hg.libsdl.org/SDL/rev/e7a79b236dc0
Correction: copysign has been supported by windows several toolchains
for a very long time, including MSVC6, MinGW, LCC-Win32, (no released
watcom versions though, but that's of no concern.)
Patch below:
diff --git a/include/SDL_config_windows.h b/include/SDL_config_windows.h
--- a/include/SDL_config_windows.h
+++ b/include/SDL_config_windows.h
@@ -142,12 +142,12 @@
#define HAVE_SQRTF 1
#define HAVE_TAN 1
#define HAVE_TANF 1
+#define HAVE_COPYSIGN 1
#if defined(_MSC_VER)
/* These functions were added with the VC++ 2013 C runtime library */
#if _MSC_VER >= 1800
#define HAVE_STRTOLL 1
#define HAVE_VSSCANF 1
-#define HAVE_COPYSIGN 1
#define HAVE_SCALBN 1
#endif
/* This function is available with at least the VC++ 2008 C runtime library */
This patch isn't relevant to this bug report. Please enter a new bug for new issues. Thanks! Are you sure that is copysign and not _copysign? There's only a _copysign symbol in msvcr100.dll and msvcr110.dll. msvcr120.dll (2013) also has the copysign symbol. I currently don't have the older compilers installed anywhere so I can't test if there's just some macro magic going on. There's also a HAVE__COPYSIGN path in SDL_stdlib.c to get the Microsoft specific function. Weird, that this isn't already defined in SDL_config_windows.h |
Created attachment 2863 [details] Patch that adds range checks to stdio_seek for 32-bit limited offsets. When RWops seeks with fseek or fseeko it uses the types long or off_t which can be 32 bits on some platforms. stdio_seek does not check if the 64-bit integer for the offset fits into a 32-bit integer. Offsets equal or larger than 2 GiB will have implementation-defined behavior and failure states would be very confusing to debug. The attached patch adds range checking by using the macros from limits.h for long type and some bit shifting for off_t because POSIX couldn't be bothered to specify min and max macros. It also defines HAVE_FSEEKI64 in SDL_config_windows.h so that the Windows function gets picked up automatically with the default config. And there's an additional error message for when ftell fails. I hope I got the detection of the limits.h header correct in the autoconf and cmake files. Does anyone know why _LARGEFILE64_SOURCE was used instead of _FILE_OFFSET_BITS=64 in RWops?