Fix static analysis code quality issues (#1582)

`libclamav/libmspack.c`: Initialize variables before first `goto done;`
to fix unitialized variable use in an error condition.

`libclamav/others.c`: Explicitly ignore return values for calls to add
JSON values when subsequent calls don't depend on them.
If we were to add error handling here, the only thing we'd do is debug-
log it. I don't think it's worth adding the extra lines of code.

`libclamav/unarj.c`: Removed dead code.
The `status` variable is immediately set afterwards based on whether or
not any files may be extracted.

`libclamav/unzip.c`: Removed dead code.
The `ret` variable is checked immediately after being set, above. This
check after the `do`-`while()` loop is dead code.

`sigtool/sigtool.c`: Fix potential NULL deref in error handling.
This is a fix for the same issue as was fixed in a previous commit.
I somehow overlooked this one. Copy/paste bug.

`libclamav/pdfdecode.c`: Fix leaked `stream` memory when
`filter_lzwdecode()` fails.

`clamdtop/clamdtop.c`: Fix possible NULL dereference if `strchr` returns
NULL in `read_version()` and `check_stats_available()`.

`libclamav/rtf.c`: Fix memory leak in `rtf_object_process()` if
`cli_gentemp_with_prefix()` fails.
Also change empty for-loop to resolve clang-format weirdness and make it
more obvious the for-loop has no body.

`libclamav/aspack.c`: Ensure that `endoff - old` is not negative in
`build_decrypt_array()` before passing to `CLI_ISCONTAINED()` which expects
unsigned values.

`libclamav/upx.c`: Fix integer overflow checks in multiple functions.

`libclamav/vba_extract.c`: Set `entries` pointer back to NULL after free in
`word_read_macro_entry()` error condition.

`libclamav/unzip.c`: Remove logic to return `CL_EMAXFILES` from
`index_local_file_headers()`. It seems it only overwrote the status when
not `CL_SUCCESS` in which case it could be overriding a more serious failure.
Further, updates to the how the ZIP parser works has made it so this needs
to return `CL_SUCCESS` in order for the caller to at least scan the files
found so far.
Finally, the calling function has checks of its own to make sure we don't
exceeds the max-files limit.

`libclamav/unzip.c`: Fix issue where `cli_append_potentially_unwanted()` in
`index_local_file_headers()` might overwrite an error in `status` with
`CL_CLEAN`. Instead, it now checks the return value and only overwrites the
`CL_EFORMAT` status with a different value if not `CL_SUCCESS`.

`libclamav/unzip.c`: Fix a potential leak with `combined_catalogue` and
`temp_catalogue` in an error condition. We should always free them if not NULL,
not just if the function failed. And to make this safe, we must set
`combined_catalogue` to NULL when we give ownership to `*catalogue`.

`libclamav/scanners.c`: Fix a potential leak in error handling for the
`cli_ole2_tempdir_scan_vba()` function.

CLAM-2768

clamdtop/clamdtop.c

@@ -1338,12 +1338,19 @@ static int read_version(conn_t *conn)
 {
     char buf[1024];
     unsigned i;
+
     if (!recv_line(conn, buf, sizeof(buf)))
         return -1;
+
     if (!strcmp(buf, "UNKNOWN COMMAND\n"))
         return -2;
+
+    char *p = strchr(buf, ':');
+    if (NULL == p)
+        return -1;
+
     // check if VERSION command is available
-    if (!strcmp(strchr(buf, ':'), ": COMMAND UNAVAILABLE\n"))
+    if (!strcmp(p, ": COMMAND UNAVAILABLE\n"))
         return -3;
 
     conn->version = strdup(buf);
@@ -1358,10 +1365,17 @@ static int check_stats_available(conn_t *conn)
 {
     char buf[1024];
     send_string(conn, "nSTATS\n");
+
     if (!recv_line(conn, buf, sizeof(buf)))
         return 0;
-    if (!strcmp(strchr(buf, ':'), ": COMMAND UNAVAILABLE\n"))
+
+    char *p = strchr(buf, ':');
+    if (NULL == p)
         return 0;
+
+    if (!strcmp(p, ": COMMAND UNAVAILABLE\n"))
+        return 0;
+
     return 1;
 }
 

libclamav/aspack.c

@@ -161,9 +161,13 @@ static uint8_t build_decrypt_array(struct ASPK *stream, uint8_t *array, uint8_t
         if (counter >= 0x10) {
             uint32_t old = endoff;
             endoff       = d3[i + 1] >> 0x10;
-            if (endoff - old) {
+
-                if (!CLI_ISCONTAINED(stream->dict_helper[which].ends, 0x100, stream->dict_helper[which].ends + old, endoff - old)) return 0;
+            if (endoff < old) return 0;
-                memset((stream->dict_helper[which].ends + old), i + 1, endoff - old);
+            uint32_t remaining = endoff - old;
+
+            if (remaining > 0) {
+                if (!CLI_ISCONTAINED(stream->dict_helper[which].ends, 0x100, stream->dict_helper[which].ends + old, remaining)) return 0;
+                memset((stream->dict_helper[which].ends + old), i + 1, remaining);
             }
         }
 

libclamav/libmspack.c

@@ -408,6 +408,9 @@ cl_error_t cli_scanmscab(cli_ctx *ctx, size_t sfx_offset)
     struct mspack_name mspack_fmap = {0};
     struct mspack_system_ex ops_ex = {0};
 
+    char *tmp_fname      = NULL;
+    bool tempfile_exists = false;
+
     mspack_fmap.fmap = ctx->fmap;
 
     if (sfx_offset > INT32_MAX) {
@@ -418,9 +421,6 @@ cl_error_t cli_scanmscab(cli_ctx *ctx, size_t sfx_offset)
 
     mspack_fmap.org = (off_t)sfx_offset;
 
-    char *tmp_fname      = NULL;
-    bool tempfile_exists = false;
-
     memset(&ops_ex, 0, sizeof(struct mspack_system_ex));
     ops_ex.ops = mspack_sys_fmap_ops;
 

libclamav/others.c

@@ -1593,21 +1593,21 @@ static cl_error_t append_virus(cli_ctx *ctx, const char *virname, IndicatorType
         if (NULL == indicator_obj) {
             cli_errmsg("append_virus: no memory for json indicator object\n");
         } else {
-            json_object_object_add(indicator_obj, "Name", json_object_new_string(virname));
+            (void)json_object_object_add(indicator_obj, "Name", json_object_new_string(virname));
             switch (type) {
                 case IndicatorType_Strong: {
-                    json_object_object_add(indicator_obj, "Type", json_object_new_string("Strong"));
+                    (void)json_object_object_add(indicator_obj, "Type", json_object_new_string("Strong"));
                 } break;
                 case IndicatorType_PotentiallyUnwanted: {
-                    json_object_object_add(indicator_obj, "Type", json_object_new_string("PotentiallyUnwanted"));
+                    (void)json_object_object_add(indicator_obj, "Type", json_object_new_string("PotentiallyUnwanted"));
                 } break;
                 case IndicatorType_Weak: {
-                    json_object_object_add(indicator_obj, "Type", json_object_new_string("Weak"));
+                    (void)json_object_object_add(indicator_obj, "Type", json_object_new_string("Weak"));
                 } break;
             }
-            json_object_object_add(indicator_obj, "Depth", json_object_new_int(0)); // 0 for this layer
+            (void)json_object_object_add(indicator_obj, "Depth", json_object_new_int(0)); // 0 for this layer
-            cli_jsonuint64(indicator_obj, "ObjectID", (uint64_t)ctx->recursion_stack[ctx->recursion_level].object_id);
+            (void)cli_jsonuint64(indicator_obj, "ObjectID", (uint64_t)ctx->recursion_stack[ctx->recursion_level].object_id);
-            json_object_array_add(indicators, indicator_obj);
+            (void)json_object_array_add(indicators, indicator_obj);
         }
 
         // If this is a strong or potentially unwanted indicator, we add it to the "Alerts" array.
@@ -1620,14 +1620,14 @@ static cl_error_t append_virus(cli_ctx *ctx, const char *virname, IndicatorType
                     status = CL_EMEM;
                     goto done;
                 }
-                json_object_object_add(ctx->this_layer_metadata_json, "Alerts", arrobj);
+                (void)json_object_object_add(ctx->this_layer_metadata_json, "Alerts", arrobj);
             }
 
             // Increment the indicator_obj reference count, so that it can be added to the "Alerts" array.
-            json_object_get(indicator_obj);
+            (void)json_object_get(indicator_obj);
 
             // Add the same indicator object to the "Alerts" array.
-            json_object_array_add(arrobj, indicator_obj);
+            (void)json_object_array_add(arrobj, indicator_obj);
         }
     }
 

libclamav/pdfdecode.c

@@ -893,6 +893,7 @@ static cl_error_t filter_lzwdecode(struct pdf_struct *pdf, struct pdf_obj *obj,
     uint8_t *content = (uint8_t *)token->content;
     uint32_t length  = token->length;
     lzw_stream stream;
+    bool stream_initialized = false;
     int echg = 1, lzwstat, rc = CL_SUCCESS;
 
     if (pdf->ctx && !(pdf->ctx->dconf->other & OTHER_CONF_LZW)) {
@@ -942,6 +943,7 @@ static cl_error_t filter_lzwdecode(struct pdf_struct *pdf, struct pdf_obj *obj,
         rc = CL_EMEM;
         goto done;
     }
+    stream_initialized = true;
 
     memset(&stream, 0, sizeof(stream));
     stream.next_in   = content;
@@ -1066,9 +1068,11 @@ static cl_error_t filter_lzwdecode(struct pdf_struct *pdf, struct pdf_obj *obj,
             break;
     }
 
-    (void)lzwInflateEnd(&stream);
-
 done:
+    if (stream_initialized) {
+        (void)lzwInflateEnd(&stream);
+    }
+
     if (rc == CL_SUCCESS) {
         if (declen == 0) {
             cli_dbgmsg("cli_pdf: empty stream after inflation completed.\n");

libclamav/rtf.c

@@ -279,8 +279,10 @@ static int rtf_object_process(struct rtf_state* state, const unsigned char* inpu
         return 0;
 
     if (data->has_partial) {
-        for (i = 0; i < len && !isxdigit(input[i]); i++)
+        for (i = 0; i < len && !isxdigit(input[i]); i++) {
-            ;
+            continue;
+        };
+
         if (i < len) {
             outdata[out_cnt++] = data->partial | hextable[input[i++]];
             data->has_partial  = 0;
@@ -532,8 +534,10 @@ int cli_scanrtf(cli_ctx* ctx)
         return CL_EMEM;
     }
 
-    if (!(tempname = cli_gentemp_with_prefix(ctx->this_layer_tmpdir, "rtf-tmp")))
+    if (!(tempname = cli_gentemp_with_prefix(ctx->this_layer_tmpdir, "rtf-tmp"))) {
+        free(stack.states);
         return CL_EMEM;
+    }
 
     if (mkdir(tempname, 0700)) {
         cli_dbgmsg("ScanRTF -> Can't create temporary directory %s\n", tempname);

libclamav/scanners.c

@@ -1826,8 +1826,8 @@ static cl_error_t cli_ole2_tempdir_scan_vba(const char *dir, cli_ctx *ctx, struc
     cl_error_t ret;
     int i, j;
     size_t data_len;
-    vba_project_t *vba_project;
+    vba_project_t *vba_project = NULL;
-    char *fullname = NULL;
+    char *fullname             = NULL;
     char vbaname[1024];
     unsigned char *data = NULL;
     char *hash;
@@ -2024,6 +2024,10 @@ done:
         free(proj_contents_fname);
     }
 
+    if (NULL != vba_project) {
+        cli_free_vba_project(vba_project);
+    }
+
     if (NULL != data) {
         free(data);
     }

libclamav/unarj.c

@@ -1209,11 +1209,8 @@ cl_error_t cli_unarj_header_check(
 
         } else if (ret == CL_BREAK) {
             cli_dbgmsg("cli_unarj_header_check: End of archive\n");
-            status = CL_BREAK;
-
         } else {
             cli_dbgmsg("cli_unarj_header_check: Error reading file header: %s\n", cl_strerror(ret));
-            status = ret;
         }
 
         CLI_FREE_AND_SET_NULL(metadata.filename);

libclamav/unzip.c

@@ -1147,11 +1147,6 @@ cl_error_t index_the_central_directory(
         }
     } while (1);
 
-    if (ret == CL_VIRUS) {
-        status = CL_VIRUS;
-        goto done;
-    }
-
     if (records_count > 1) {
         /*
          * Sort the records by local file offset
@@ -1463,7 +1458,6 @@ cl_error_t index_local_file_headers(
     struct zip_record *prev_record        = NULL;
     size_t local_file_headers_count       = 0;
     uint32_t num_overlapping_files        = 0;
-    bool exceeded_max_files               = false;
 
     if (NULL == catalogue || NULL == num_records || NULL == *catalogue) {
         cli_dbgmsg("index_local_file_headers: Invalid NULL arguments\n");
@@ -1505,8 +1499,6 @@ cl_error_t index_local_file_headers(
         if (ctx->engine->maxfiles && total_files_found >= ctx->engine->maxfiles) {
             cli_dbgmsg("cli_unzip: Files limit reached (max: %u)\n", ctx->engine->maxfiles);
             cli_append_potentially_unwanted_if_heur_exceedsmax(ctx, "Heuristics.Limits.Exceeded.MaxFiles");
-            exceeded_max_files = true; // Set a bool so we can return the correct status code later.
-                                       // We still need to scan the files we found while reviewing the file records up to this limit.
             break;
         }
 
@@ -1611,10 +1603,12 @@ cl_error_t index_local_file_headers(
                         cli_dbgmsg("    current file start: %u\n", curr_record->local_header_offset);
 
                         if (ZIP_MAX_NUM_OVERLAPPING_FILES < num_overlapping_files) {
+                            status = CL_EFORMAT;
                             if (SCAN_HEURISTICS) {
-                                status = cli_append_potentially_unwanted(ctx, "Heuristics.Zip.OverlappingFiles");
+                                ret = cli_append_potentially_unwanted(ctx, "Heuristics.Zip.OverlappingFiles");
-                            } else {
+                                if (CL_SUCCESS != ret) {
-                                status = CL_EFORMAT;
+                                    status = ret;
+                                }
                             }
                             goto done;
                         }
@@ -1631,8 +1625,11 @@ cl_error_t index_local_file_headers(
 
         free(temp_catalogue);
         temp_catalogue = NULL;
+
         free(*catalogue);
-        *catalogue   = combined_catalogue;
+        *catalogue         = combined_catalogue;
+        combined_catalogue = NULL;
+
         *num_records = total_files_found;
     } else {
         free(temp_catalogue);
@@ -1654,34 +1651,30 @@ done:
             free(*catalogue);
             *catalogue = NULL;
         }
+    }
 
-        if (NULL != temp_catalogue) {
+    if (NULL != temp_catalogue) {
-            size_t i;
+        size_t i;
-            for (i = 0; i < local_file_headers_count; i++) {
+        for (i = 0; i < local_file_headers_count; i++) {
-                if (NULL != temp_catalogue[i].original_filename) {
+            if (NULL != temp_catalogue[i].original_filename) {
-                    free(temp_catalogue[i].original_filename);
+                free(temp_catalogue[i].original_filename);
-                    temp_catalogue[i].original_filename = NULL;
+                temp_catalogue[i].original_filename = NULL;
-                }
             }
-            free(temp_catalogue);
-            temp_catalogue = NULL;
         }
+        free(temp_catalogue);
+        temp_catalogue = NULL;
+    }
 
-        if (NULL != combined_catalogue) {
+    if (NULL != combined_catalogue) {
-            size_t i;
+        size_t i;
-            for (i = 0; i < total_files_found; i++) {
+        for (i = 0; i < total_files_found; i++) {
-                if (NULL != combined_catalogue[i].original_filename) {
+            if (NULL != combined_catalogue[i].original_filename) {
-                    free(combined_catalogue[i].original_filename);
+                free(combined_catalogue[i].original_filename);
-                    combined_catalogue[i].original_filename = NULL;
+                combined_catalogue[i].original_filename = NULL;
-                }
             }
-            free(combined_catalogue);
-            combined_catalogue = NULL;
-        }
-
-        if (exceeded_max_files) {
-            status = CL_EMAXFILES;
         }
+        free(combined_catalogue);
+        combined_catalogue = NULL;
     }
 
     return status;

libclamav/upx.c

@@ -360,7 +360,7 @@ int upx_inflate2b(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u
             } while ((oob = doubleebx(src, &myebx, &scur, ssize)) == 0);
             if (oob == -1)
                 return -1;
-            if (backsize + 2 > UINT32_MAX)
+            if (backsize > UINT32_MAX - 2)
                 return -1;
             backsize += 2;
         }
@@ -455,7 +455,7 @@ int upx_inflate2d(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u
             } while ((oob = doubleebx(src, &myebx, &scur, ssize)) == 0);
             if (oob == -1)
                 return -1;
-            if (backsize + 2 > UINT32_MAX)
+            if (backsize > UINT32_MAX - 2)
                 return -1;
             backsize += 2;
         }
@@ -554,7 +554,7 @@ int upx_inflate2e(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u
                 } while ((oob = doubleebx(src, &myebx, &scur, ssize)) == 0);
                 if (oob == -1)
                     return -1;
-                if (backsize + 2 > UINT32_MAX)
+                if (backsize > UINT32_MAX - 2)
                     return -1;
                 backsize += 2;
             }
@@ -563,7 +563,7 @@ int upx_inflate2e(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u
         if ((uint32_t)unp_offset < 0xfffffb00)
             backsize++;
 
-        if (backsize + 2 > UINT32_MAX)
+        if (backsize > UINT32_MAX - 2)
             return -1;
         backsize += 2;
 

libclamav/vba_extract.c

@@ -2075,6 +2075,7 @@ word_read_macro_info(int fd, macro_info_t *macro_info)
     }
     if (!word_read_macro_entry(fd, macro_info)) {
         free(macro_info->entries);
+        macro_info->entries = NULL;
         macro_info->count = 0;
         return NULL;
     }

sigtool/sigtool.c

@@ -517,10 +517,10 @@ done:
             if (NULL != ctx->options) {
                 free(ctx->options);
             }
-            if (ctx->recursion_stack[ctx->recursion_level].evidence) {
-                evidence_free(ctx->recursion_stack[ctx->recursion_level].evidence);
-            }
             if (NULL != ctx->recursion_stack) {
+                if (ctx->recursion_stack[ctx->recursion_level].evidence) {
+                    evidence_free(ctx->recursion_stack[ctx->recursion_level].evidence);
+                }
                 free(ctx->recursion_stack);
             }
             free(ctx);