From aa6ce542099fac7ccadbd39de254e834927fb5bc Mon Sep 17 00:00:00 2001 From: Andreas Schiffler Date: Wed, 13 Mar 2013 08:35:03 -0700 Subject: [PATCH] Fix bug 122 - SDL_RWops bug fixes: set RWops.type field, add input validation, add test coverage --- include/SDL_rwops.h | 8 ++ src/file/SDL_rwops.c | 26 ++++- test/testautomation_rwops.c | 195 +++++++++++++++++++++++++++++++++--- 3 files changed, 215 insertions(+), 14 deletions(-) diff --git a/include/SDL_rwops.h b/include/SDL_rwops.h index 68c8e5f0b..61c30920e 100644 --- a/include/SDL_rwops.h +++ b/include/SDL_rwops.h @@ -40,6 +40,14 @@ extern "C" { /* *INDENT-ON* */ #endif +/* RWops Types */ +#define SDL_RWOPS_UNKNOWN 0 /* Unknown stream type */ +#define SDL_RWOPS_WINFILE 1 /* Win32 file */ +#define SDL_RWOPS_STDFILE 2 /* Stdio file */ +#define SDL_RWOPS_JNIFILE 3 /* Android asset */ +#define SDL_RWOPS_MEMORY 4 /* Memory stream */ +#define SDL_RWOPS_MEMORY_RO 5 /* Read-Only memory stream */ + /** * This is the read/write operation structure -- very basic. */ diff --git a/src/file/SDL_rwops.c b/src/file/SDL_rwops.c index 55f601e34..99010c3b7 100644 --- a/src/file/SDL_rwops.c +++ b/src/file/SDL_rwops.c @@ -513,6 +513,7 @@ SDL_RWFromFile(const char *file, const char *mode) rwops->read = Android_JNI_FileRead; rwops->write = Android_JNI_FileWrite; rwops->close = Android_JNI_FileClose; + rwops->type = SDL_RWOPS_JNIFILE; #elif defined(__WIN32__) rwops = SDL_AllocRW(); @@ -527,6 +528,7 @@ SDL_RWFromFile(const char *file, const char *mode) rwops->read = windows_file_read; rwops->write = windows_file_write; rwops->close = windows_file_close; + rwops->type = SDL_RWOPS_WINFILE; #elif HAVE_STDIO_H { @@ -570,6 +572,7 @@ SDL_RWFromFP(FILE * fp, SDL_bool autoclose) rwops->close = stdio_close; rwops->hidden.stdio.fp = fp; rwops->hidden.stdio.autoclose = autoclose; + rwops->type = SDL_RWOPS_STDFILE; } return (rwops); } @@ -585,7 +588,15 @@ SDL_RWFromFP(void * fp, SDL_bool autoclose) SDL_RWops * SDL_RWFromMem(void *mem, int size) { - SDL_RWops *rwops; + SDL_RWops *rwops = NULL; + if (!mem) { + SDL_InvalidParamError("mem"); + return (rwops); + } + if (!size) { + SDL_InvalidParamError("size"); + return (rwops); + } rwops = SDL_AllocRW(); if (rwops != NULL) { @@ -597,6 +608,7 @@ SDL_RWFromMem(void *mem, int size) rwops->hidden.mem.base = (Uint8 *) mem; rwops->hidden.mem.here = rwops->hidden.mem.base; rwops->hidden.mem.stop = rwops->hidden.mem.base + size; + rwops->type = SDL_RWOPS_MEMORY; } return (rwops); } @@ -604,7 +616,15 @@ SDL_RWFromMem(void *mem, int size) SDL_RWops * SDL_RWFromConstMem(const void *mem, int size) { - SDL_RWops *rwops; + SDL_RWops *rwops = NULL; + if (!mem) { + SDL_InvalidParamError("mem"); + return (rwops); + } + if (!size) { + SDL_InvalidParamError("size"); + return (rwops); + } rwops = SDL_AllocRW(); if (rwops != NULL) { @@ -616,6 +636,7 @@ SDL_RWFromConstMem(const void *mem, int size) rwops->hidden.mem.base = (Uint8 *) mem; rwops->hidden.mem.here = rwops->hidden.mem.base; rwops->hidden.mem.stop = rwops->hidden.mem.base + size; + rwops->type = SDL_RWOPS_MEMORY_RO; } return (rwops); } @@ -629,6 +650,7 @@ SDL_AllocRW(void) if (area == NULL) { SDL_OutOfMemory(); } + area->type = SDL_RWOPS_UNKNOWN; return (area); } diff --git a/test/testautomation_rwops.c b/test/testautomation_rwops.c index 101dccb2a..be93809db 100644 --- a/test/testautomation_rwops.c +++ b/test/testautomation_rwops.c @@ -21,16 +21,18 @@ const char* RWopsReadTestFilename = "rwops_read"; const char* RWopsWriteTestFilename = "rwops_write"; +const char* RWopsAlphabetFilename = "rwops_alphabet"; static const char RWopsHelloWorldTestString[] = "Hello World!"; static const char RWopsHelloWorldCompString[] = "Hello World!"; +static const char RWopsAlphabetString[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; /* Fixture */ void RWopsSetUp(void *arg) { - int fileLen = SDL_strlen(RWopsHelloWorldTestString); + int fileLen; FILE *handle; int writtenLen; int result; @@ -38,18 +40,32 @@ RWopsSetUp(void *arg) /* Clean up from previous runs (if any); ignore errors */ remove(RWopsReadTestFilename); remove(RWopsWriteTestFilename); + remove(RWopsAlphabetFilename); /* Create a test file */ handle = fopen(RWopsReadTestFilename, "w"); SDLTest_AssertCheck(handle != NULL, "Verify creation of file '%s' returned non NULL handle", RWopsReadTestFilename); if (handle == NULL) return; - /* Write some known test into it */ + /* Write some known text into it */ + fileLen = SDL_strlen(RWopsHelloWorldTestString); writtenLen = (int)fwrite(RWopsHelloWorldTestString, 1, fileLen, handle); SDLTest_AssertCheck(fileLen == writtenLen, "Verify number of written bytes, expected %i, got %i", fileLen, writtenLen); result = fclose(handle); SDLTest_AssertCheck(result == 0, "Verify result from fclose, expected 0, got %i", result); + /* Create a second test file */ + handle = fopen(RWopsAlphabetFilename, "w"); + SDLTest_AssertCheck(handle != NULL, "Verify creation of file '%s' returned non NULL handle", RWopsAlphabetFilename); + if (handle == NULL) return; + + /* Write alphabet text into it */ + fileLen = SDL_strlen(RWopsAlphabetString); + writtenLen = (int)fwrite(RWopsAlphabetString, 1, fileLen, handle); + SDLTest_AssertCheck(fileLen == writtenLen, "Verify number of written bytes, expected %i, got %i", fileLen, writtenLen); + result = fclose(handle); + SDLTest_AssertCheck(result == 0, "Verify result from fclose, expected 0, got %i", result); + SDLTest_AssertPass("Creation of test file completed"); } @@ -62,6 +78,8 @@ RWopsTearDown(void *arg) result = remove(RWopsReadTestFilename); SDLTest_AssertCheck(result == 0, "Verify result from remove(%s), expected 0, got %i", RWopsReadTestFilename, result); remove(RWopsWriteTestFilename); + result = remove(RWopsAlphabetFilename); + SDLTest_AssertCheck(result == 0, "Verify result from remove(%s), expected 0, got %i", RWopsAlphabetFilename, result); SDLTest_AssertPass("Cleanup of test files completed"); } @@ -137,6 +155,14 @@ _testGenericRWopsValidations(SDL_RWops *rw, int write) "Verify seek to -1 with SDL_RWseek (RW_SEEK_END), expected %i, got %i", sizeof(RWopsHelloWorldTestString)-2, i); + + /* Invalid whence seek */ + i = SDL_RWseek( rw, 0, 999 ); + SDLTest_AssertPass("Call to SDL_RWseek(...,0,invalid_whence) succeeded"); + SDLTest_AssertCheck( + i == (Sint64)(-1), + "Verify seek with SDL_RWseek (invalid_whence); expected: -1, got %i", + i); } /*! @@ -171,6 +197,18 @@ rwops_testParamNegative (void) SDLTest_AssertPass("Call to SDL_RWFromFile(\"something\", NULL) succeeded"); SDLTest_AssertCheck(rwops == NULL, "Verify SDL_RWFromFile(\"something\", NULL) returns NULL"); + rwops = SDL_RWFromMem((void *)NULL, 10); + SDLTest_AssertPass("Call to SDL_RWFromMem(NULL, 10) succeeded"); + SDLTest_AssertCheck(rwops == NULL, "Verify SDL_RWFromMem(NULL, 10) returns NULL"); + + rwops = SDL_RWFromMem((void *)RWopsAlphabetString, 0); + SDLTest_AssertPass("Call to SDL_RWFromMem(data, 0) succeeded"); + SDLTest_AssertCheck(rwops == NULL, "Verify SDL_RWFromMem(data, 0) returns NULL"); + + rwops = SDL_RWFromConstMem((const void *)RWopsAlphabetString, 0); + SDLTest_AssertPass("Call to SDL_RWFromConstMem(data, 0) succeeded"); + SDLTest_AssertCheck(rwops == NULL, "Verify SDL_RWFromConstMem(data, 0) returns NULL"); + return TEST_COMPLETED; } @@ -178,13 +216,14 @@ rwops_testParamNegative (void) * @brief Tests opening from memory. * * \sa http://wiki.libsdl.org/moin.cgi/SDL_RWFromMem - * http://wiki.libsdl.org/moin.cgi/SDL_RWClose + * \sa http://wiki.libsdl.org/moin.cgi/SDL_RWClose */ int rwops_testMem (void) { char mem[sizeof(RWopsHelloWorldTestString)]; SDL_RWops *rw; + int result; /* Clear buffer */ SDL_zero(mem); @@ -197,12 +236,16 @@ rwops_testMem (void) /* Bail out if NULL */ if (rw == NULL) return TEST_ABORTED; + /* Check type */ + SDLTest_AssertCheck(rw->type == SDL_RWOPS_MEMORY, "Verify RWops type is SDL_RWOPS_MEMORY; expected: %d, got: %d", SDL_RWOPS_MEMORY, rw->type); + /* Run generic tests */ _testGenericRWopsValidations(rw, 1); /* Close */ - SDL_RWclose(rw); + result = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); return TEST_COMPLETED; } @@ -219,6 +262,7 @@ int rwops_testConstMem (void) { SDL_RWops *rw; + int result; /* Open handle */ rw = SDL_RWFromConstMem( RWopsHelloWorldCompString, sizeof(RWopsHelloWorldCompString)-1 ); @@ -228,12 +272,16 @@ rwops_testConstMem (void) /* Bail out if NULL */ if (rw == NULL) return TEST_ABORTED; + /* Check type */ + SDLTest_AssertCheck(rw->type == SDL_RWOPS_MEMORY_RO, "Verify RWops type is SDL_RWOPS_MEMORY_RO; expected: %d, got: %d", SDL_RWOPS_MEMORY_RO, rw->type); + /* Run generic tests */ _testGenericRWopsValidations( rw, 0 ); /* Close handle */ - SDL_RWclose(rw); + result = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); return TEST_COMPLETED; } @@ -250,6 +298,7 @@ int rwops_testFileRead(void) { SDL_RWops *rw; + int result; /* Read test. */ rw = SDL_RWFromFile(RWopsReadTestFilename, "r"); @@ -259,12 +308,28 @@ rwops_testFileRead(void) // Bail out if NULL if (rw == NULL) return TEST_ABORTED; + /* Check type */ +#if defined(ANDROID) + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_STDFILE || rw->type == SDL_RWOPS_JNIFILE, + "Verify RWops type is SDL_RWOPS_STDFILE or SDL_RWOPS_JNIFILE; expected: %d|%d, got: %d", SDL_RWOPS_STDFILE, SDL_RWOPS_JNIFILE, rw->type); +#elif defined(__WIN32__) + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_WINFILE, + "Verify RWops type is SDL_RWOPS_WINFILE; expected: %d, got: %d", SDL_RWOPS_WINFILE, rw->type); +#else + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_STDFILE, + "Verify RWops type is SDL_RWOPS_STDFILE; expected: %d, got: %d", SDL_RWOPS_STDFILE, rw->type); +#endif + /* Run generic tests */ _testGenericRWopsValidations( rw, 0 ); /* Close handle */ - SDL_RWclose(rw); + result = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); return TEST_COMPLETED; } @@ -280,6 +345,7 @@ int rwops_testFileWrite(void) { SDL_RWops *rw; + int result; /* Write test. */ rw = SDL_RWFromFile(RWopsWriteTestFilename, "w+"); @@ -289,12 +355,28 @@ rwops_testFileWrite(void) // Bail out if NULL if (rw == NULL) return TEST_ABORTED; + /* Check type */ +#if defined(ANDROID) + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_STDFILE || rw->type == SDL_RWOPS_JNIFILE, + "Verify RWops type is SDL_RWOPS_STDFILE or SDL_RWOPS_JNIFILE; expected: %d|%d, got: %d", SDL_RWOPS_STDFILE, SDL_RWOPS_JNIFILE, rw->type); +#elif defined(__WIN32__) + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_WINFILE, + "Verify RWops type is SDL_RWOPS_WINFILE; expected: %d, got: %d", SDL_RWOPS_WINFILE, rw->type); +#else + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_STDFILE, + "Verify RWops type is SDL_RWOPS_STDFILE; expected: %d, got: %d", SDL_RWOPS_STDFILE, rw->type); +#endif + /* Run generic tests */ _testGenericRWopsValidations( rw, 1 ); /* Close handle */ - SDL_RWclose(rw); + result = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); return TEST_COMPLETED; } @@ -313,6 +395,7 @@ rwops_testFPRead(void) { FILE *fp; SDL_RWops *rw; + int result; /* Run read tests. */ fp = fopen(RWopsReadTestFilename, "r"); @@ -332,12 +415,18 @@ rwops_testFPRead(void) return TEST_ABORTED; } + /* Check type */ + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_STDFILE, + "Verify RWops type is SDL_RWOPS_STDFILE; expected: %d, got: %d", SDL_RWOPS_STDFILE, rw->type); + /* Run generic tests */ _testGenericRWopsValidations( rw, 0 ); /* Close handle - does fclose() */ - SDL_RWclose(rw); + result = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); return TEST_COMPLETED; } @@ -356,6 +445,7 @@ rwops_testFPWrite(void) { FILE *fp; SDL_RWops *rw; + int result; /* Run write tests. */ fp = fopen(RWopsWriteTestFilename, "w+"); @@ -375,12 +465,18 @@ rwops_testFPWrite(void) return TEST_ABORTED; } + /* Check type */ + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_STDFILE, + "Verify RWops type is SDL_RWOPS_STDFILE; expected: %d, got: %d", SDL_RWOPS_STDFILE, rw->type); + /* Run generic tests */ _testGenericRWopsValidations( rw, 1 ); /* Close handle - does fclose() */ - SDL_RWclose(rw); + result = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); return TEST_COMPLETED; } @@ -399,6 +495,11 @@ rwops_testAllocFree (void) SDLTest_AssertPass("Call to SDL_AllocRW() succeeded"); SDLTest_AssertCheck(rw != NULL, "Validate result from SDL_AllocRW() is not NULL"); if (rw==NULL) return TEST_ABORTED; + + /* Check type */ + SDLTest_AssertCheck( + rw->type == SDL_RWOPS_UNKNOWN, + "Verify RWops type is SDL_RWOPS_UNKNOWN; expected: %d, got: %d", SDL_RWOPS_UNKNOWN, rw->type); /* Free context again */ SDL_FreeRW(rw); @@ -407,6 +508,72 @@ rwops_testAllocFree (void) return TEST_COMPLETED; } +/** + * @brief Compare memory and file reads + * + * \sa http://wiki.libsdl.org/moin.cgi/SDL_RWFromMem + * \sa http://wiki.libsdl.org/moin.cgi/SDL_RWFromFile + */ +int +rwops_testCompareRWFromMemWithRWFromFile(void) +{ + int slen = 26; + char buffer_file[27]; + char buffer_mem[27]; + size_t rv_file; + size_t rv_mem; + Uint64 sv_file; + Uint64 sv_mem; + SDL_RWops* rwops_file; + SDL_RWops* rwops_mem; + int size; + int result; + + + for (size=5; size<10; size++) + { + /* Terminate buffer */ + buffer_file[slen] = 0; + buffer_mem[slen] = 0; + + /* Read/seek from memory */ + rwops_mem = SDL_RWFromMem((void *)RWopsAlphabetString, slen); + SDLTest_AssertPass("Call to SDL_RWFromMem()"); + rv_mem = SDL_RWread(rwops_mem, buffer_mem, size, 6); + SDLTest_AssertPass("Call to SDL_RWread(mem, size=%d)", size); + sv_mem = SDL_RWseek(rwops_mem, 0, SEEK_END); + SDLTest_AssertPass("Call to SDL_RWseek(mem,SEEK_END)"); + result = SDL_RWclose(rwops_mem); + SDLTest_AssertPass("Call to SDL_RWclose(mem)"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); + + /* Read/see from file */ + rwops_file = SDL_RWFromFile(RWopsAlphabetFilename, "r"); + SDLTest_AssertPass("Call to SDL_RWFromFile()"); + rv_file = SDL_RWread(rwops_file, buffer_file, size, 6); + SDLTest_AssertPass("Call to SDL_RWread(file, size=%d)", size); + sv_file = SDL_RWseek(rwops_file, 0, SEEK_END); + SDLTest_AssertPass("Call to SDL_RWseek(file,SEEK_END)"); + result = SDL_RWclose(rwops_file); + SDLTest_AssertPass("Call to SDL_RWclose(file)"); + SDLTest_AssertCheck(result == 0, "Verify result value is 0; got: %d", result); + + /* Compare */ + SDLTest_AssertCheck(rv_mem == rv_file, "Verify returned read blocks matches for mem and file reads; got: rv_mem=%d rv_file=%d", rv_mem, rv_file); + SDLTest_AssertCheck(sv_mem == sv_file, "Verify SEEK_END position matches for mem and file seeks; got: sv_mem=%llu sv_file=%llu", sv_mem, sv_file); + SDLTest_AssertCheck(buffer_mem[slen] == 0, "Verify mem buffer termination; expected: 0, got: %d", buffer_mem[slen]); + SDLTest_AssertCheck(buffer_file[slen] == 0, "Verify file buffer termination; expected: 0, got: %d", buffer_file[slen]); + SDLTest_AssertCheck( + SDL_strncmp(buffer_mem, RWopsAlphabetString, slen) == 0, + "Verify mem buffer contain alphabet string; expected: %s, got: %s", RWopsAlphabetString, buffer_mem); + SDLTest_AssertCheck( + SDL_strncmp(buffer_file, RWopsAlphabetString, slen) == 0, + "Verify file buffer contain alphabet string; expected: %s, got: %s", RWopsAlphabetString, buffer_file); + } + + return TEST_COMPLETED; +} + /** * @brief Tests writing and reading from file using endian aware functions. * @@ -435,6 +602,7 @@ rwops_testFileWriteReadEndian(void) Uint16 LE16test; Uint32 LE32test; Uint64 LE64test; + int cresult; for (mode = 0; mode < 3; mode++) { @@ -523,9 +691,9 @@ rwops_testFileWriteReadEndian(void) SDLTest_AssertCheck(LE64test == LE64value, "Validate return value from SDL_ReadLE64, expected: %llu, got: %llu", LE64value, LE64test); /* Close handle */ - SDL_RWclose(rw); + cresult = SDL_RWclose(rw); SDLTest_AssertPass("Call to SDL_RWclose() succeeded"); - + SDLTest_AssertCheck(cresult == 0, "Verify result value is 0; got: %d", cresult); } return TEST_COMPLETED; @@ -562,10 +730,13 @@ static const SDLTest_TestCaseReference rwopsTest8 = static const SDLTest_TestCaseReference rwopsTest9 = { (SDLTest_TestCaseFp)rwops_testFileWriteReadEndian, "rwops_testFileWriteReadEndian", "Test writing and reading via the Endian aware functions", TEST_ENABLED }; +static const SDLTest_TestCaseReference rwopsTest10 = + { (SDLTest_TestCaseFp)rwops_testCompareRWFromMemWithRWFromFile, "rwops_testCompareRWFromMemWithRWFromFile", "Compare RWFromMem and RWFromFile RWops for read and seek", TEST_ENABLED }; + /* Sequence of RWops test cases */ static const SDLTest_TestCaseReference *rwopsTests[] = { &rwopsTest1, &rwopsTest2, &rwopsTest3, &rwopsTest4, &rwopsTest5, &rwopsTest6, - &rwopsTest7, &rwopsTest8, &rwopsTest9, NULL + &rwopsTest7, &rwopsTest8, &rwopsTest9, &rwopsTest10, NULL }; /* RWops test suite (global) */