Merge pull request #250 from awslabs/win-fix-rc

Add readFile fix on Windows to release

src/common/include/com/amazonaws/kinesis/video/common/CommonDefs.h

@@ -922,12 +922,23 @@ extern PUBLIC_API atomicXor globalAtomicXor;
 #ifndef FREAD
 #define FREAD fread
 #endif
+#ifndef FEOF
+#define FEOF feof
+#endif
 #ifndef FSEEK
+#if defined _WIN32 || defined _WIN64
+#define FSEEK _fseeki64
+#else
 #define FSEEK fseek
 #endif
+#endif
 #ifndef FTELL
+#if defined _WIN32 || defined _WIN64
+#define FTELL _ftelli64
+#else
 #define FTELL ftell
 #endif
+#endif
 #ifndef FREMOVE
 #define FREMOVE remove
 #endif

src/utils/src/FileIo.c

@@ -23,6 +23,11 @@ STATUS readFile(PCHAR filePath, BOOL binMode, PBYTE pBuffer, PUINT64 pSize)
     CHK(fp != NULL, STATUS_OPEN_FILE_FAILED);
 
     // Get the size of the file
+
+    // Use Windows-specific _fseeki64 and _ftelli64 as the traditional fseek and ftell are non-compliant on systems that do not provide
+    // the same guarantees as POSIX. On these systems, setting the file position indicator to the
+    // end of the file using fseek() is not guaranteed to work for a binary stream, and consequently,
+    // the amount of memory allocated may be incorrect, leading to a potential vulnerability.
     FSEEK(fp, 0, SEEK_END);
     fileLen = FTELL(fp);
 
@@ -37,7 +42,14 @@ STATUS readFile(PCHAR filePath, BOOL binMode, PBYTE pBuffer, PUINT64 pSize)
 
     // Read the file into memory buffer
     FSEEK(fp, 0, SEEK_SET);
-    CHK(FREAD(pBuffer, (SIZE_T) fileLen, 1, fp) == 1, STATUS_READ_FILE_FAILED);
+
+    // fread would either return 1, i.e, the number of objects we've requested it to read
+    // or it would run into end-of-file / error.
+    // fread does not distinguish between end-of-file and error,
+    // and callers must use feof and ferror to determine which occurred.
+    if (FREAD(pBuffer, (SIZE_T) fileLen, 1, fp) != 1) {
+        CHK(FEOF(fp), STATUS_READ_FILE_FAILED);
+    }
 
 CleanUp:
 
@@ -75,6 +87,11 @@ STATUS readFileSegment(PCHAR filePath, BOOL binMode, PBYTE pBuffer, UINT64 offse
     CHK(fp != NULL, STATUS_OPEN_FILE_FAILED);
 
     // Get the size of the file
+
+    // Use Windows-specific _fseeki64 and _ftelli64 as the traditional fseek and ftell are non-compliant on systems that do not provide
+    // the same guarantees as POSIX. On these systems, setting the file position indicator to the
+    // end of the file using fseek() is not guaranteed to work for a binary stream, and consequently,
+    // the amount of memory allocated may be incorrect, leading to a potential vulnerability.
     FSEEK(fp, 0, SEEK_END);
     fileLen = FTELL(fp);
 
@@ -83,7 +100,16 @@ STATUS readFileSegment(PCHAR filePath, BOOL binMode, PBYTE pBuffer, UINT64 offse
 
     // Set the offset and read the file content
     result = FSEEK(fp, (UINT32) offset, SEEK_SET);
-    CHK(result == 0 && (FREAD(pBuffer, (SIZE_T) readSize, 1, fp) == 1), STATUS_READ_FILE_FAILED);
+
+    CHK(result == 0, STATUS_READ_FILE_FAILED);
+
+    // fread would either return 1, i.e, the number of objects we've requested it to read
+    // or it would run into end-of-file / error.
+    // fread does not distinguish between end-of-file and error,
+    // and callers must use feof and ferror to determine which occurred.
+    if (FREAD(pBuffer, (SIZE_T) readSize, 1, fp) != 1) {
+        CHK(FEOF(fp), STATUS_READ_FILE_FAILED);
+    }
 
 CleanUp:
 

tst/client/ClientTestFixture.h

@@ -72,7 +72,6 @@
 #define TEST_DEFAULT_PRODUCER_CONFIG_FRAME_SIZE 50000
 #define TEST_DEFAULT_PRODUCER_CONFIG_FRAME_RATE 20
 
-
 #define PASS_TEST_FOR_ZERO_RETENTION_AND_OFFLINE()                                                                                                   \
     if ((mStreamInfo.retention == 0 || !mStreamInfo.streamCaps.fragmentAcks) && mStreamInfo.streamCaps.streamingType == STREAMING_TYPE_OFFLINE) {    \
         EXPECT_EQ(STATUS_OFFLINE_MODE_WITH_ZERO_RETENTION, createKinesisVideoStreamSync(mClientHandle, &mStreamInfo, &mStreamHandle));               \

tst/utils/FileIo.cpp

@@ -0,0 +1,159 @@
+#include "UtilTestFixture.h"
+
+#ifdef _WIN32
+#define TEST_TEMP_DIR_PATH                                      (PCHAR) "C:\\Windows\\Temp\\"
+#define TEST_TEMP_DIR_PATH_NO_ENDING_SEPARTOR                   (PCHAR) "C:\\Windows\\Temp"
+#else
+#define TEST_TEMP_DIR_PATH                                      (PCHAR) "/tmp/"
+#define TEST_TEMP_DIR_PATH_NO_ENDING_SEPARTOR                   (PCHAR) "/tmp"
+#endif
+
+class FileIoFunctionalityTest : public UtilTestBase {
+};
+
+class FileIoUnitTest : public UtilTestBase {
+};
+
+TEST_F(FileIoUnitTest, readFile_filePathNull) {
+    EXPECT_EQ(STATUS_NULL_ARG, readFile(NULL, TRUE, NULL, NULL));
+    EXPECT_EQ(STATUS_NULL_ARG, readFile(NULL, FALSE, NULL, NULL));
+}
+
+TEST_F(FileIoUnitTest, readFile_sizeNull) {
+    EXPECT_EQ(STATUS_NULL_ARG, readFile((PCHAR) (TEST_TEMP_DIR_PATH "test"), TRUE, NULL, NULL));
+    EXPECT_EQ(STATUS_NULL_ARG, readFile((PCHAR) (TEST_TEMP_DIR_PATH "test"), FALSE, NULL, NULL));
+}
+
+TEST_F(FileIoUnitTest, readFile_asciiBufferTooSmall) {
+    FILE *file = FOPEN((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), "w");
+    UINT64 fileSize = 43;
+    PCHAR fileBuffer = NULL;
+
+    fprintf(file, "This is line 1\nThis is line 2\nThis is line 3\n");
+    FCLOSE(file);
+
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), FALSE, NULL, &fileSize));
+
+// In Windows systems, the newline character is represented by a combination of 
+// two characters: Carriage Return (CR) followed by Line Feed (LF), written as \r\n. 
+// So, each newline character in a text file on Windows contributes two bytes to the file size.
+#if defined _WIN32 || defined _WIN64    
+    EXPECT_EQ(48, fileSize);
+#else
+    EXPECT_EQ(45, fileSize);
+#endif
+
+    fileSize = 43;
+
+    fileBuffer = (PCHAR) MEMCALLOC(1, (fileSize + 1) * SIZEOF(CHAR));
+    EXPECT_EQ(STATUS_BUFFER_TOO_SMALL, readFile((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), FALSE, (PBYTE) fileBuffer, &fileSize));
+
+    remove((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"));
+    SAFE_MEMFREE(fileBuffer);
+}
+
+TEST_F(FileIoUnitTest, readFile_binaryBufferTooSmall) {
+    FILE *file = fopen((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), "wb");
+    UINT64 fileSize = 43;
+    PCHAR fileBuffer = NULL;
+    CHAR data[100] = "This is line 1\nThis is line 2\nThis is line 3\n";
+
+    FWRITE(data, SIZEOF(CHAR), STRLEN(data), file);
+    FCLOSE(file);
+
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), TRUE, NULL, &fileSize));
+    EXPECT_EQ(45, fileSize);
+
+    fileSize = 43;
+
+    fileBuffer = (PCHAR) MEMCALLOC(1, (fileSize + 1) * SIZEOF(CHAR));
+    EXPECT_EQ(STATUS_BUFFER_TOO_SMALL, readFile((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), TRUE, (PBYTE) fileBuffer, &fileSize));
+
+    remove((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"));
+    SAFE_MEMFREE(fileBuffer);
+}
+
+TEST_F(FileIoFunctionalityTest, readFile_asciiFileSize) {
+    FILE *file = FOPEN((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), "w");
+    CHAR fileBuffer[256];
+    UINT64 fileSize = ARRAY_SIZE(fileBuffer);
+
+    fprintf(file, "This is line 1\nThis is line 2\nThis is line 3\n");
+    FCLOSE(file);
+
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), FALSE, NULL, &fileSize));
+
+// In Windows systems, the newline character is represented by a combination of 
+// two characters: Carriage Return (CR) followed by Line Feed (LF), written as \r\n. 
+// So, each newline character in a text file on Windows contributes two bytes to the file size.
+#if defined _WIN32 || defined _WIN64    
+    EXPECT_EQ(48, fileSize);
+#else
+    EXPECT_EQ(45, fileSize);
+#endif
+
+    remove((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"));
+}
+
+TEST_F(FileIoFunctionalityTest, readFile_binaryFileSize) {
+    FILE *file = fopen((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), "wb");
+    CHAR fileBuffer[256];
+    CHAR data[100] = "This is line 1\nThis is line 2\nThis is line 3\n";
+    UINT64 fileSize = ARRAY_SIZE(fileBuffer);
+
+    FWRITE(data, SIZEOF(CHAR), STRLEN(data), file);
+    FCLOSE(file);
+
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR)(TEST_TEMP_DIR_PATH "binarytest"), TRUE, NULL, &fileSize));
+    EXPECT_EQ(45, fileSize);
+
+    remove((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"));
+}
+
+TEST_F(FileIoFunctionalityTest, readFile_binary) {
+    FILE *file = fopen((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), "wb");
+    UINT64 fileSize;
+    PCHAR fileBuffer = NULL;
+    CHAR data[100] = "This is line 1\nThis is line 2\nThis is line 3\n";
+
+    FWRITE(data, SIZEOF(CHAR), STRLEN(data), file);
+    FCLOSE(file);
+
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), TRUE, NULL, &fileSize));
+    EXPECT_EQ(45, fileSize);
+
+    fileBuffer = (PCHAR) MEMCALLOC(1, (fileSize + 1) * SIZEOF(CHAR));
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"), TRUE, (PBYTE) fileBuffer, &fileSize));
+    EXPECT_STREQ(data, fileBuffer);
+
+    remove((PCHAR) (TEST_TEMP_DIR_PATH "binarytest"));
+    SAFE_MEMFREE(fileBuffer);
+}
+
+TEST_F(FileIoFunctionalityTest, readFile_ascii) {
+    FILE *file = fopen((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), "w");
+    UINT64 fileSize;
+    PCHAR fileBuffer = NULL;
+    CHAR data[100] = "This is line 1\nThis is line 2\nThis is line 3\n";
+
+    FWRITE(data, SIZEOF(CHAR), STRLEN(data), file);
+    FCLOSE(file);
+
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), FALSE, NULL, &fileSize));
+
+// In Windows systems, the newline character is represented by a combination of 
+// two characters: Carriage Return (CR) followed by Line Feed (LF), written as \r\n. 
+// So, each newline character in a text file on Windows contributes two bytes to the file size.
+#if defined _WIN32 || defined _WIN64    
+    EXPECT_EQ(48, fileSize);
+#else
+    EXPECT_EQ(45, fileSize);
+#endif
+
+    fileBuffer = (PCHAR) MEMCALLOC(1, (fileSize + 1) * SIZEOF(CHAR));
+    EXPECT_EQ(STATUS_SUCCESS, readFile((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"), FALSE, (PBYTE) fileBuffer, &fileSize));
+    EXPECT_STREQ(data, fileBuffer);
+
+    remove((PCHAR) (TEST_TEMP_DIR_PATH "asciitest"));
+    SAFE_MEMFREE(fileBuffer);
+}