From: Oliver Kurth <okurth@vmware.com>
Date: Wed, 30 Oct 2019 18:18:21 +0000 (-0700)
Subject: Suppress a couple of coverity false alarms in FileLoggerOpen().
X-Git-Tag: stable-11.1.0~185
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ba1408a50387a78cfd9fa45b788edb6f81e46a89;p=thirdparty%2Fopen-vm-tools.git

Suppress a couple of coverity false alarms in FileLoggerOpen().

The stat() system call is used to determine whether to rotate logs.
There is no danger of time-of-check vs. time-of-use because the rotation
decision still holds even under the very-unlikely condition that the existing
log file size changes.

When rotating the logs, the service should not stop when a rename() fails
on an old file.  The process ignores the rename() return code intentionally.
The error condition cannot be logged because the process is already in the
log handling context and would either crash or risk a recursion loop
otherwise.  In addition, writing to stdout/stderr is useless, since the
process is running as a service and the stdout/stderr is reopened on /dev/null.

Therefore, the above two coverity issues are suppressed in the code.
---

diff --git a/open-vm-tools/lib/glibUtils/fileLogger.c b/open-vm-tools/lib/glibUtils/fileLogger.c
index ace171d1e..65f4e7f98 100644
--- a/open-vm-tools/lib/glibUtils/fileLogger.c
+++ b/open-vm-tools/lib/glibUtils/fileLogger.c
@@ -247,6 +247,15 @@ FileLoggerOpen(FileLogger *data)
       struct stat fstats;
 #endif
 
+      /*
+       * In order to determine whether we should rotate the logs,
+       * we are calling the system call stat() to get the existing log file
+       * size.
+       * The time of check vs. time of use issue does not apply to this use
+       * case, as even the file size is increasing, it will not affect the log
+       * rotation decision. So Suppress the fs_check_call coverity warning.
+       */
+      /* coverity[fs_check_call] */
       if (g_stat(path, &fstats) > -1) {
          data->logSize = (gint) fstats.st_size;
       }
@@ -283,6 +292,13 @@ FileLoggerOpen(FileLogger *data)
             if (!g_file_test(dest, G_FILE_TEST_IS_DIR) &&
                 (!g_file_test(dest, G_FILE_TEST_EXISTS) ||
                  g_unlink(dest) == 0)) {
+               /*
+                * We should ignore an unlikely rename() system call failure,
+                * as we should keep our service running with non-critical errors.
+                * We cannot log the error because we are already in the log
+                * handler context to avoid crash or recursive logging loop.
+                */
+               /* coverity[check_return] */
                g_rename(src, dest);
             } else {
                g_unlink(src);
@@ -311,6 +327,7 @@ FileLoggerOpen(FileLogger *data)
 #ifdef _WIN32
       (void) Win32Access_SetFileOwnerRW(path);
 #else
+      /* coverity[toctou] */
       (void) chmod(path, 0600);
 #endif
 #endif // VMX86_TOOLS
diff --git a/open-vm-tools/vgauth/service/fileLogger.c b/open-vm-tools/vgauth/service/fileLogger.c
index eca231d89..e38007700 100644
--- a/open-vm-tools/vgauth/service/fileLogger.c
+++ b/open-vm-tools/vgauth/service/fileLogger.c
@@ -76,6 +76,15 @@ ServiceFileLoggerOpen(FileLoggerData *data)
       /* GStatBuf was added in 2.26. */
       GStatBuf fstats;
 
+      /*
+       * In order to determine whether we should rotate the logs,
+       * we are calling the system call stat() to get the existing log file
+       * size.
+       * The time of check vs. time of use issue does not apply to this use
+       * case, as even the file size is increasing, it will not affect the log
+       * rotation decision. So Suppress the fs_check_call coverity warning.
+       */
+      /* coverity[fs_check_call] */
       if (g_stat(path, &fstats) > -1) {
          g_atomic_int_set(&data->logSize, (gint) fstats.st_size);
       }
@@ -112,6 +121,13 @@ ServiceFileLoggerOpen(FileLoggerData *data)
             if (!g_file_test(dest, G_FILE_TEST_IS_DIR) &&
                 (!g_file_test(dest, G_FILE_TEST_EXISTS) ||
                  g_unlink(dest) == 0)) {
+               /*
+                * We should ignore an unlikely rename() system call failure,
+                * as we should keep our service running with non-critical errors.
+                * We cannot log the error because we are already in the log
+                * handler context to avoid crash or recursive logging loop.
+                */
+               /* coverity[check_return] */
                g_rename(src, dest);
             } else {
                g_unlink(src);
@@ -128,6 +144,7 @@ ServiceFileLoggerOpen(FileLoggerData *data)
       }
    }
 
+   /* coverity[toctou] */
    logfile = g_fopen(path, data->append ? "a" : "w");
    /*
     * Make log readable only by root/Administrator.  Just log any error;