ClamDScan: --fdpass/--stream leak; ExcludePath issues

ClamDScan will leak the memory for the scan target filename if using
`--fdpass` or using `--stream`. This commit fixes that leak.
Resolves: https://bugzilla.clamav.net/show_bug.cgi?id=12648

ClamDScan will fail to scan any file after running into an
"ExcludePath" exclusion when using `--fdpass` or `--stream` AND
--multiscan (-m). The issue is because the parallel_callback()
callback function used by file tree walk (ftw) feature returns an
error code for excluded files rather than "success".
Memory for the accidentally-excluded paths for a given directory also
appears to be leaked.
This commit resolves this accidental-abort issue and the memory leak.

There was an additional single file path memory leak when using
`--fdpass` caused by bad error handling in `cli_ftw()`.
This was fixed by removing the confusing ternaries, and using
separate pointers for each filename copy.

ClamDScan with ExcludePath regex may fail to exclude absolute paths
when performing relative scans because the exclude-check function may
match using provided relative path (E.g. `/some/path/../another/path`)
rather than an absolute path (E.g. `/some/path/another/path`).
This issue is resolved by getting the real path at the start of the
scan, eliminating `.` and `..` relative pathing from all filepaths.

TODO 1: In addition to being recursive (bad for stack safety), the
File Tree Walk (FTW) implementation is a spaghetti code and should
be refactored.

TODO 2: ExcludePath will print out "Excluded" for each path that is
excluded when using `--fdpass` or `--stream`, and for each path
directly scanned that is directly excluded. But in a recursive
regular-scan, the "Excluded" message for the those paths is missing.

clamdscan/client.c

@@ -309,7 +309,21 @@ static char *makeabs(const char *basepath)
 static int client_scan(const char *file, int scantype, int *infected, int *err, int maxlevel, int session, int flags)
 {
     int ret;
-    char *fullpath = makeabs(file);
+    char *real_path = NULL;
+    char *fullpath  = NULL;
+
+    /* Convert relative path to fullpath */
+    fullpath = makeabs(file);
+
+    /* Convert fullpath to the real path (evaluating symlinks and . and ..).
+       Doing this early on will ensure that the scan results will appear consistent
+       across regular scans, --fdpass scans, and --stream scans. */
+    if (CL_SUCCESS != cli_realpath(fullpath, &real_path)) {
+        logg("*client_scan: Failed to determine real filename of %s.\n", fullpath);
+    } else {
+        free(fullpath);
+        fullpath = real_path;
+    }
 
     if (!fullpath)
         return 0;

clamdscan/proto.c

@@ -238,10 +238,13 @@ static int send_fdpass(int sockd, const char *filename)
 /* 0: scan, 1: skip */
 static int chkpath(const char *path)
 {
+    int status = 0;
     const struct optstruct *opt;
+    char *real_path = NULL;
 
     if (!path) {
-        return 1;
+        status = 1;
+        goto done;
     }
 
     if ((opt = optget(clamdopts, "ExcludePath"))->enabled) {
@@ -249,12 +252,18 @@ static int chkpath(const char *path)
             if (match_regex(path, opt->strarg) == 1) {
                 if (printinfected != 1)
                     logg("~%s: Excluded\n", path);
-                return 1;
+                status = 1;
+                goto done;
             }
             opt = opt->nextarg;
         }
     }
-    return 0;
+
+done:
+    if (NULL != real_path) {
+        free(real_path);
+    }
+    return status;
 }
 
 static int ftw_chkpath(const char *path, struct cli_ftw_cbdata *data)
@@ -444,6 +453,7 @@ static cl_error_t serial_callback(STATBUF *sb, char *filename, const char *path,
     }
 
     if (chkpath(path)) {
+        /* Exclude the path */
         status = CL_SUCCESS;
         goto done;
     }
@@ -498,6 +508,9 @@ static cl_error_t serial_callback(STATBUF *sb, char *filename, const char *path,
 
     status = CL_SUCCESS;
 done:
+    if (NULL != real_filename) {
+        free(real_filename);
+    }
     free(filename);
     return status;
 }
@@ -624,12 +637,14 @@ static cl_error_t parallel_callback(STATBUF *sb, char *filename, const char *pat
             logg("*Failed to determine real filename of %s.\n", filename);
             logg("*Quarantine of the file may fail if file path contains symlinks.\n");
         } else {
-            free(filename);
+            free(filename); /* callback is responsible for free'ing filename parameter. */
             filename = real_filename;
         }
     }
 
     if (chkpath(filename)) {
+        /* Exclude the path */
+        status = CL_SUCCESS;
         goto done;
     }
     c->files++;

libclamav/others.h

@@ -922,35 +922,56 @@ struct cli_ftw_cbdata {
     void *data;
 };
 
-/*
- * return CL_BREAK to break out without an error, CL_SUCCESS to continue,
- * or any CL_E* to break out due to error.
+/**
+ * @brief Callback to process each file in a file tree walk (FTW).
+ *
  * The callback is responsible for freeing filename when it is done using it.
+ *
  * Note that callback decides if directory traversal should continue
  * after an error, we call the callback with reason == error,
  * and if it returns CL_BREAK we break.
+ *
+ * Return:
+ * - CL_BREAK to break out without an error,
+ * - CL_SUCCESS to continue,
+ * - any CL_E* to break out due to error.
  */
 typedef cl_error_t (*cli_ftw_cb)(STATBUF *stat_buf, char *filename, const char *path, enum cli_ftw_reason reason, struct cli_ftw_cbdata *data);
 
-/*
- * returns 1 if the path should be skipped and 0 otherwise
- * uses callback data
+/**
+ * @brief Callback to determine if a path in a file tree walk (FTW) should be skipped.
+ * Has access to the same callback data as the main FTW callback function (above).
+ *
+ * Return:
+ * - 1 if the path should be skipped (i.e. to not call the callback for the given path),
+ * - 0 if the path should be processed (i.e. to call the callback for the given path).
  */
 typedef int (*cli_ftw_pathchk)(const char *path, struct cli_ftw_cbdata *data);
 
-/*
- * returns
- *  CL_SUCCESS if it traversed all files and subdirs
- *  CL_BREAK if traversal has stopped at some point
- *  CL_E* if error encountered during traversal and we had to break out
+/**
+ * @brief Traverse a file path, calling the callback function on each file
+ * within if the pathchk() check allows for it. Will skip certain file types:
+ * -
+ *
  * This is regardless of virus found/not, that is the callback's job to store.
  * Note that the callback may dispatch async the scan, so that when cli_ftw
  * returns we don't know the infected/notinfected status of the directory yet!
+ *
  * Due to this if the callback scans synchronously it should store the infected
  * status in its cbdata.
  * This works for both files and directories. It stats the path to determine
  * which one it is.
  * If it is a file, it simply calls the callback once, otherwise recurses.
+ *
+ * @param base      The top level directory (or file) path to be processed
+ * @param flags     A bitflag field for the CLI_FTW_* flag options (see above)
+ * @param maxdepth  The max recursion depth.
+ * @param callback  The cli_ftw_cb callback to invoke on each file AND directory.
+ * @param data      Callback data for the callback function.
+ * @param pathchk   A function used to determine if the callback should be run on the given file.
+ * @return cl_error_t CL_SUCCESS if it traversed all files and subdirs
+ * @return cl_error_t CL_BREAK if traversal has stopped at some point
+ * @return cl_error_t CL_E* if error encountered during traversal and we had to break out
  */
 cl_error_t cli_ftw(char *base, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk);
 

libclamav/others_common.c

@@ -574,6 +574,20 @@ static int get_filetype(const char *fname, int flags, int need_stat,
     return stated;
 }
 
+/**
+ * @brief Determine the file type and pass the metadata to the callback as the "reason".
+ *
+ * The callback may end up doing something or doing nothing, depending on the reason.
+ *
+ * @param fname         The file path
+ * @param flags         CLI_FTW_* bitflag field
+ * @param statbuf       [out] the stat metadata for the file.
+ * @param stated        [out] 1 if statbuf contains stat info, 0 if not. -1 if there was a stat error.
+ * @param ft            [out] will indicate if the file was skipped based on the file type.
+ * @param callback      the callback (E.g. function that may scan the file)
+ * @param data          callback data
+ * @return cl_error_t
+ */
 static cl_error_t handle_filetype(const char *fname, int flags,
                                   STATBUF *statbuf, int *stated, enum filetype *ft,
                                   cli_ftw_cb callback, struct cli_ftw_cbdata *data)
@@ -616,7 +630,7 @@ done:
     return status;
 }
 
-static int cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk);
+static cl_error_t cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk);
 static int handle_entry(struct dirent_data *entry, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk)
 {
     if (!entry->is_dir) {
@@ -630,10 +644,11 @@ cl_error_t cli_ftw(char *path, int flags, int maxdepth, cli_ftw_cb callback, str
 {
     cl_error_t status = CL_EMEM;
     STATBUF statbuf;
-    enum filetype ft = ft_unknown;
-    struct dirent_data entry;
-    int stated      = 0;
-    char *path_copy = NULL;
+    enum filetype ft               = ft_unknown;
+    struct dirent_data entry       = {0};
+    int stated                     = 0;
+    char *filename_for_callback    = NULL;
+    char *filename_for_handleentry = NULL;
 
     if (((flags & CLI_FTW_TRIM_SLASHES) || pathchk) && path[0] && path[1]) {
         char *pathend;
@@ -652,11 +667,14 @@ cl_error_t cli_ftw(char *path, int flags, int maxdepth, cli_ftw_cb callback, str
         goto done;
     }
 
+    /* Determine if the file should be skipped (special file or symlink).
+       This will also get the stat metadata. */
     status = handle_filetype(path, flags, &statbuf, &stated, &ft, callback, data);
     if (status != CL_SUCCESS) {
         goto done;
     }
 
+    /* Bail out if the file should be skipped. */
     if (ft_skipped(ft)) {
         status = CL_SUCCESS;
         goto done;
@@ -665,38 +683,63 @@ cl_error_t cli_ftw(char *path, int flags, int maxdepth, cli_ftw_cb callback, str
     entry.statbuf = stated ? &statbuf : NULL;
     entry.is_dir  = ft == ft_directory;
 
+    /*
+     * handle_entry() doesn't call the callback for directories, so we'll call it now first.
+     */
     if (entry.is_dir) {
-        path_copy = cli_strdup(path);
-        if (NULL == path_copy) {
+        /* Allocate the filename for the callback function. TODO: this FTW code is spaghetti, refactor. */
+        filename_for_callback = cli_strdup(path);
+        if (NULL == filename_for_callback) {
             goto done;
         }
 
-        status = callback(entry.statbuf, path_copy, path, visit_directory_toplev, data);
+        status = callback(entry.statbuf, filename_for_callback, path, visit_directory_toplev, data);
+
+        filename_for_callback = NULL; // free'd by the callback
+
         if (status != CL_SUCCESS) {
             goto done;
         }
     }
 
-    path_copy = cli_strdup(path);
-    if (NULL == path_copy) {
-        goto done;
-    }
-
-    entry.filename = entry.is_dir ? NULL : path_copy;
-    entry.dirname  = entry.is_dir ? path : NULL;
+    /*
+     * Now call handle_entry() to either call the callback for files,
+     * or recurse deeper into the file tree walk.
+     * TODO: Recursion is bad, this whole thing should be iterative
+     */
+    if (entry.is_dir) {
+        entry.dirname = path;
+    } else {
+        /* Allocate the filename for the callback function within the handle_entry function. TODO: this FTW code is spaghetti, refactor. */
+        filename_for_handleentry = cli_strdup(path);
+        if (NULL == filename_for_handleentry) {
+            goto done;
+        }
 
+        entry.filename = filename_for_handleentry;
+    }
     status = handle_entry(&entry, flags, maxdepth, callback, data, pathchk);
 
+    filename_for_handleentry = NULL; // free'd by the callback call in handle_entry()
+
 done:
+    if (NULL != filename_for_callback) {
+        /* Free-check just in case someone injects additional calls and error handling before callback(). */
+        free(filename_for_callback);
+    }
+    if (NULL != filename_for_handleentry) {
+        /* Free-check just in case someone injects additional calls and error handling before handle_entry(). */
+        free(filename_for_handleentry);
+    }
     return status;
 }
 
-static int cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk)
+static cl_error_t cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk)
 {
     DIR *dd;
     struct dirent_data *entries = NULL;
     size_t i, entries_cnt = 0;
-    int ret;
+    cl_error_t ret;
 
     if (maxdepth < 0) {
         /* exceeded recursion limit */
@@ -824,8 +867,11 @@ static int cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb
                     free(entry->filename);
                 if (entry->statbuf)
                     free(entry->statbuf);
-                if (ret != CL_SUCCESS)
+                if (ret != CL_SUCCESS) {
+                    /* Something went horribly wrong, Skip the rest of the files */
+                    cli_errmsg("File tree walk aborted.\n");
                     break;
+                }
             }
             for (i++; i < entries_cnt; i++) {
                 struct dirent_data *entry = &entries[i];