From 1755f43fffa58d7ded5dfb96270d619c3ee1c43d Mon Sep 17 00:00:00 2001 From: Ilan Truanovsky Date: Tue, 24 Jan 2023 07:04:45 -0800 Subject: [PATCH 1/3] Fix NEGATIVE_RETURNS Coverity issue lib/pkg_editor/src/pkg_editor.c:500:3: Type: Improper use of negative value (NEGATIVE_RETURNS) lib/pkg_editor/src/pkg_editor.c:489:3: 1. path: Condition "f == NULL", taking false branch. lib/pkg_editor/src/pkg_editor.c:496:3: 2. negative_return_fn: Function "ftell(f)" returns a negative number. lib/pkg_editor/src/pkg_editor.c:496:3: 3. assign: Assigning: "file_size" = "ftell(f)". lib/pkg_editor/src/pkg_editor.c:500:3: 4. negative_returns: "file_size" is passed to a parameter that cannot be negative. lib/pkg_editor/src/pkg_editor.c:461:3: 4.1. path: Condition "buf == NULL", taking false branch. lib/pkg_editor/src/pkg_editor.c:459:60: 4.2. sizet: "size" is a size_t parameter. --- lib/pkg_editor/src/pkg_editor.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/pkg_editor/src/pkg_editor.c b/lib/pkg_editor/src/pkg_editor.c index 3b27216b..1a91c894 100644 --- a/lib/pkg_editor/src/pkg_editor.c +++ b/lib/pkg_editor/src/pkg_editor.c @@ -493,7 +493,13 @@ static char *read_file_into_buffer(struct acl_pkg_file *pkg, // get file size fseek(f, 0, SEEK_END); - file_size = ftell(f); + const long ftell_return = ftell(f); + if (ftell_return < 0) { + fprintf(stderr, "Couldn't determine size of file %s\n", in_file); + fclose(f); + return NULL; + } + file_size = (size_t)ftell_return; rewind(f); // slurp the whole file into allocated buf From 70fa51ac28d9aacb783572b38e9e9068f7bf075a Mon Sep 17 00:00:00 2001 From: Ilan Truanovsky Date: Tue, 24 Jan 2023 07:11:18 -0800 Subject: [PATCH 2/3] Fix OVERRUN Coverity issues lib/pkg_editor/src/pkg_editor.c:1641:5: Type: Out-of-bounds read (OVERRUN) lib/pkg_editor/src/pkg_editor.c:1584:3: 1. path: Condition "buffer != NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1585:5: 2. path: Condition "input == NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1585:5: 3. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1588:3: 4. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1594:3: 5. path: Condition "ret != 0", taking false branch. lib/pkg_editor/src/pkg_editor.c:1610:3: 6. path: Condition "z_info.strm.avail_in > 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1612:5: 7. path: Condition "!read_data(&info, 20UL /* sizeof (info) */, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1617:5: 8. path: Condition "info.magic != 3203399403U", taking false branch. lib/pkg_editor/src/pkg_editor.c:1625:5: 9. path: Condition "info.kind == PACK_END", taking false branch. lib/pkg_editor/src/pkg_editor.c:1630:5: 10. path: Condition "!read_data(name, info.name_length, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1638:5: 11. path: Condition "12288UL /* 3 * 4096 */ < out_dir_length", taking true branch. lib/pkg_editor/src/pkg_editor.c:1638:5: 12. cond_at_least: Checking "12288UL < out_dir_length" implies that "out_dir_length" is at least 12289 on the true branch. lib/pkg_editor/src/pkg_editor.c:1641:5: 13. overrun-local: Overrunning array of 12288 bytes at byte offset 12290 by dereferencing pointer "full_name + out_dir_length + 1". [Note: The source code implementation of the function has been overridden by a builtin model.] lib/pkg_editor/src/pkg_editor.c:1411:9: Type: Out-of-bounds read (OVERRUN) lib/pkg_editor/src/pkg_editor.c:1323:3: 1. path: Condition "!append_data(&info, 20UL /* sizeof (info) */, z_info, of, 0)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1330:3: 2. path: Condition "!append_data(dir_name, name_length, z_info, of, 0)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1385:5: 3. path: Condition "8192UL /* 2 * 4096 */ < name_length", taking false branch. lib/pkg_editor/src/pkg_editor.c:1385:5: 4. cond_at_most: Checking "8192UL < name_length" implies that "info.name_length" and "name_length" may be up to 8192 on the false branch. lib/pkg_editor/src/pkg_editor.c:1398:5: 5. path: Condition "dir == NULL", taking false branch. lib/pkg_editor/src/pkg_editor.c:1404:5: 6. path: Condition "entry", taking true branch. lib/pkg_editor/src/pkg_editor.c:1406:7: 7. path: Condition "strcmp(entry->d_name, ".") != 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1406:7: 8. path: Condition "strcmp(entry->d_name, "..") != 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1411:9: 9. overrun-local: Overrunning array of 8192 bytes at byte offset 8192 by dereferencing pointer "full_name + name_length". [Note: The source code implementation of the function has been overridden by a builtin model.] --- lib/pkg_editor/src/pkg_editor.c | 68 +++++++++++++++------------------ 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/lib/pkg_editor/src/pkg_editor.c b/lib/pkg_editor/src/pkg_editor.c index 1a91c894..cc8640b2 100644 --- a/lib/pkg_editor/src/pkg_editor.c +++ b/lib/pkg_editor/src/pkg_editor.c @@ -1313,9 +1313,6 @@ static acl_pack_kind add_file(const char *out_file, FILE *of, const char *file, static acl_pack_kind add_directory(const char *out_file, FILE *of, const char *dir_name, ZInfo *z_info) { -#ifdef FULL_NAME_LENGTH -#undef FULL_NAME_LENGTH -#endif acl_pkg_pack_info info; size_t name_length = strlen(dir_name) + 1; @@ -1341,24 +1338,31 @@ static acl_pack_kind add_directory(const char *out_file, FILE *of, // Now walk the directory processing each name. { + #ifdef _WIN32 #define FULL_NAME_LENGTH (2 * MAX_PATH) +#else +#define FULL_NAME_LENGTH (2 * PATH_MAX) +#endif + char full_name[FULL_NAME_LENGTH]; - if (FULL_NAME_LENGTH < name_length) { + + // Full name must be large enough to store dir_name plus a trailing path + // separator + if (name_length + 1 > FULL_NAME_LENGTH) { fprintf(stderr, "acl_pkg_pack: Failed to write to %s: %s\n", out_file, "Directory name too long"); return PACK_END; } +#ifdef _WIN32 HANDLE file_handle; WIN32_FIND_DATA file_info; // Partially initialize the full path name. - strncpy(full_name, dir_name, FULL_NAME_LENGTH); + strncpy(full_name, dir_name, FULL_NAME_LENGTH - 1); strncpy(full_name + name_length - 1, "\\*.*", - FULL_NAME_LENGTH - name_length + 1); - if (full_name[FULL_NAME_LENGTH - 1] != '\0') { - full_name[FULL_NAME_LENGTH - 1] = '\0'; - } + FULL_NAME_LENGTH - name_length); + full_name[FULL_NAME_LENGTH - 1] = '\0'; // Walk through all the files in the directory. file_handle = FindFirstFile(full_name, &file_info); @@ -1372,10 +1376,9 @@ static acl_pack_kind add_directory(const char *out_file, FILE *of, // Finish the full file name strncpy(full_name + name_length, file_info.cFileName, - FULL_NAME_LENGTH - name_length); - if (full_name[FULL_NAME_LENGTH - 1] != '\0') { - full_name[FULL_NAME_LENGTH - 1] = '\0'; - } + FULL_NAME_LENGTH - name_length - 1); + full_name[FULL_NAME_LENGTH - 1] = '\0'; + if (add_file_or_dir(out_file, of, full_name, z_info) == PACK_END) { FindClose(file_handle); return PACK_END; @@ -1386,14 +1389,6 @@ static acl_pack_kind add_directory(const char *out_file, FILE *of, // Linux DIR *dir; struct dirent *entry; -#define FULL_NAME_LENGTH (2 * PATH_MAX) - char full_name[FULL_NAME_LENGTH]; - if (FULL_NAME_LENGTH < name_length) { - fprintf(stderr, "acl_pkg_pack: Failed to write to %s: %s\n", out_file, - "Directory name too long"); - return PACK_END; - } - // Partially initialize the full path name. strncpy(full_name, dir_name, FULL_NAME_LENGTH - 1); full_name[FULL_NAME_LENGTH - 1] = '\0'; @@ -1414,10 +1409,9 @@ static acl_pack_kind add_directory(const char *out_file, FILE *of, // Finish the full file name, truncate name if the file is too long to // avoid buffer overflow size_t buffer_space_left = FULL_NAME_LENGTH - name_length; - strncpy(full_name + name_length, entry->d_name, buffer_space_left); - if (full_name[FULL_NAME_LENGTH - 1] != '\0') { - full_name[FULL_NAME_LENGTH - 1] = '\0'; - } + strncpy(full_name + name_length, entry->d_name, buffer_space_left - 1); + full_name[FULL_NAME_LENGTH - 1] = '\0'; + if (add_file_or_dir(out_file, of, full_name, z_info) == PACK_END) { closedir(dir); return PACK_END; @@ -1427,6 +1421,7 @@ static acl_pack_kind add_directory(const char *out_file, FILE *of, } closedir(dir); #endif +#undef FULL_NAME_LENGTH } return PACK_DIR; } @@ -1561,12 +1556,6 @@ static int read_data(void *data, size_t size, ZInfo *z_info, FILE *in_fd) { static int acl_pkg_unpack_buffer_or_file(const char *buffer, size_t buffer_size, FILE *input, const char *out_dir, const char *routine_name) { -#ifdef FULL_NAME_LEN -#undef FULL_NAME_LEN -#endif -#ifdef NAME_LEN -#undef NAME_LEN -#endif #ifdef _WIN32 #define FULL_NAME_LEN (3 * MAX_PATH) #define NAME_LEN (2 * MAX_PATH) @@ -1641,15 +1630,18 @@ static int acl_pkg_unpack_buffer_or_file(const char *buffer, size_t buffer_size, return 0; } - // Generate the full name, truncate or zero pad to avoid buffer overflow - if (FULL_NAME_LEN < out_dir_length) { + // Generate the full name, truncate or zero pad to avoid buffer overflow. + // FULL_NAME_LEN must be large enough to store out_dir and a trailing path + // separator and null terminator + if (out_dir_length + 2 > FULL_NAME_LEN) { fprintf(stderr, "%s: Directory name too long\n", routine_name); + inflateEnd(&z_info.strm); + return 0; } + strncpy(full_name + out_dir_length + 1, name, - FULL_NAME_LEN - out_dir_length - 1); - if (full_name[FULL_NAME_LEN - 1] != '\0') { - full_name[FULL_NAME_LEN - 1] = '\0'; - } + FULL_NAME_LEN - out_dir_length - 2); + full_name[FULL_NAME_LEN - 1] = '\0'; if (info.kind == PACK_DIR) { #ifdef _WIN32 @@ -1718,6 +1710,8 @@ static int acl_pkg_unpack_buffer_or_file(const char *buffer, size_t buffer_size, inflateEnd(&z_info.strm); return 1; +#undef FULL_NAME_LEN +#undef NAME_LEN } int acl_pkg_unpack_buffer(const char *buffer, size_t buffer_size, From e032cf1f48dfc244962594b9dec04cb92cfe5335 Mon Sep 17 00:00:00 2001 From: Ilan Truanovsky Date: Tue, 24 Jan 2023 07:15:43 -0800 Subject: [PATCH 3/3] Fixed CHECK_RETURN Coverity issues lib/pkg_editor/src/pkg_editor.c:1604:3: Type: Unchecked return value from library (CHECKED_RETURN) lib/pkg_editor/src/pkg_editor.c:1584:3: Unchecked call to function 1. path: Condition "buffer != NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1585:5: 2. path: Condition "input == NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1585:5: 3. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1588:3: 4. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1594:3: 5. path: Condition "ret != 0", taking false branch. lib/pkg_editor/src/pkg_editor.c:1604:3: 6. check_return: Calling "mkdir(full_name, 493U)" without checking return value. This library function may fail and return an error code. lib/pkg_editor/src/pkg_editor.c:1651:7: Type: Unchecked return value from library (CHECKED_RETURN) lib/pkg_editor/src/pkg_editor.c:1584:3: Unchecked call to function 1. path: Condition "buffer != NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1585:5: 2. path: Condition "input == NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1585:5: 3. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1588:3: 4. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1594:3: 5. path: Condition "ret != 0", taking false branch. lib/pkg_editor/src/pkg_editor.c:1610:3: 6. path: Condition "z_info.strm.avail_in > 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1612:5: 7. path: Condition "!read_data(&info, 20UL /* sizeof (info) */, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1617:5: 8. path: Condition "info.magic != 3203399403U", taking false branch. lib/pkg_editor/src/pkg_editor.c:1625:5: 9. path: Condition "info.kind == PACK_END", taking false branch. lib/pkg_editor/src/pkg_editor.c:1630:5: 10. path: Condition "!read_data(name, info.name_length, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1638:5: 11. path: Condition "12288UL /* 3 * 4096 */ < out_dir_length", taking true branch. lib/pkg_editor/src/pkg_editor.c:1643:5: 12. path: Condition "full_name[12287 /* 3 * 4096 - 1 */] != 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1647:5: 13. path: Condition "info.kind == PACK_DIR", taking true branch. lib/pkg_editor/src/pkg_editor.c:1651:7: 14. check_return: Calling "mkdir(full_name, 493U)" without checking return value. This library function may fail and return an error code. --- lib/pkg_editor/src/pkg_editor.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/pkg_editor/src/pkg_editor.c b/lib/pkg_editor/src/pkg_editor.c index cc8640b2..6f4e8b38 100644 --- a/lib/pkg_editor/src/pkg_editor.c +++ b/lib/pkg_editor/src/pkg_editor.c @@ -1514,6 +1514,17 @@ int acl_pkg_pack(const char *out_file, const char **input_files_dirs) { return 1 /* success */; } +static void create_dir(const char *dir_name) { + // Create output directory. We can ignore the error output since it will + // only delay the failure to the first attempt of creating a file in the + // (not) newly created directory. +#ifdef _WIN32 + (void)CreateDirectory(dir_name, NULL); +#else + (void)mkdir(dir_name, 0755); +#endif +} + static int read_data(void *data, size_t size, ZInfo *z_info, FILE *in_fd) { // We want to fill 'data' with 'size' bytes. z_info->strm.next_out = data; @@ -1593,12 +1604,7 @@ static int acl_pkg_unpack_buffer_or_file(const char *buffer, size_t buffer_size, return 0; } - // Create output directory (ignore any errors). -#ifdef _WIN32 - CreateDirectory(full_name, NULL); -#else - mkdir(full_name, 0755); -#endif + create_dir(full_name); full_name[out_dir_length] = '/'; // Process the file until we hit the PACK_END record (or finish the @@ -1644,11 +1650,7 @@ static int acl_pkg_unpack_buffer_or_file(const char *buffer, size_t buffer_size, full_name[FULL_NAME_LEN - 1] = '\0'; if (info.kind == PACK_DIR) { -#ifdef _WIN32 - CreateDirectory(full_name, NULL); -#else - mkdir(full_name, 0755); -#endif + create_dir(full_name); } else { // Read file contents FILE *out_file = fopen(full_name, "wb");