Merge pull request #3833 from nrwahl2/nrwahl2-coverity

pacemaker-remoted load_env_var refactors, and assorted minor changes

Author: clumens

configure.ac

@@ -1099,7 +1099,6 @@ dnl ========================================================================
 cc_temp_flags "$CFLAGS $WERROR"
 
 # Optional headers (inclusion of these should be conditional in C code)
-AC_CHECK_HEADERS([linux/swab.h])
 AC_CHECK_HEADERS([sys/signalfd.h])
 AC_CHECK_HEADERS([uuid/uuid.h])
 AC_CHECK_HEADERS([security/pam_appl.h pam/pam_appl.h])

daemons/execd/remoted_pidone.c

@@ -61,135 +61,152 @@ static struct {
 
 /*!
  * \internal
- * \brief Check a line of text for a valid environment variable name
+ * \brief Check whether a string is a valid environment variable name
  *
- * \param[in]  line  Text to check
- * \param[out] first  First character of valid name if found, NULL otherwise
- * \param[out] last   Last character of valid name if found, NULL otherwise
+ * \param[in] name  String to check
  *
- * \return TRUE if valid name found, FALSE otherwise
+ * \return \c true if \p name is a valid name, or \c false otherwise
  * \note It's reasonable to impose limitations on environment variable names
  *       beyond what C or setenv() does: We only allow names that contain only
  *       [a-zA-Z0-9_] characters and do not start with a digit.
  */
 static bool
-find_env_var_name(char *line, char **first, char **last)
+valid_env_var_name(const gchar *name)
 {
-    // Skip leading whitespace
-    *first = line;
-    while (isspace(**first)) {
-        ++*first;
+    if (!isalpha(*name) && (*name != '_')) {
+        // Invalid first character
+        return false;
     }
 
-    if (isalpha(**first) || (**first == '_')) { // Valid first character
-        *last = *first;
-        while (isalnum(*(*last + 1)) || (*(*last + 1) == '_')) {
-            ++*last;
+    // The rest of the characters must be alphanumeric or underscores
+    for (name++; isalnum(*name) || (*name == '_'); name++);
+    return *name == '\0';
+}
+
+/*!
+ * \internal
+ * \brief Read one environment variable assignment and set the value
+ *
+ * Empty lines and trailing comments are ignored. This function handles
+ * backslashes, single quotes, and double quotes in a manner similar to a POSIX
+ * shell.
+ *
+ * This function has at least two limitations compared to a shell:
+ * * An assignment must be contained within a single line.
+ * * Only one assignment per line is supported.
+ *
+ * It would be possible to get rid of these limitations, but it doesn't seem
+ * worth the trouble of implementation and testing.
+ *
+ * \param[in] line  Line containing an environment variable assignment statement
+ */
+static void
+load_env_var_line(const char *line)
+{
+    gint argc = 0;
+    gchar **argv = NULL;
+    GError *error = NULL;
+
+    gchar *name = NULL;
+    gchar *value = NULL;
+
+    int rc = pcmk_rc_ok;
+    const char *reason = NULL;
+    const char *value_to_set = NULL;
+
+    /* g_shell_parse_argv() does the following in a manner similar to the shell:
+     * * tokenizes the value
+     * * strips a trailing '#' comment if one exists
+     * * handles backslashes, single quotes, and double quotes
+     */
+
+    // Ensure the line contains zero or one token besides an optional comment
+    if (!g_shell_parse_argv(line, &argc, NULL, &error)) {
+        // Empty line (or only space/comment) means nothing to do and no error
+        if (error->code != G_SHELL_ERROR_EMPTY_STRING) {
+            reason = error->message;
         }
-        return TRUE;
+        goto done;
+    }
+    if (argc != 1) {
+        // "argc != 1" for sanity; should imply "argc > 1" by now
+        reason = "line contains garbage";
+        goto done;
+    }
+
+    rc = pcmk__scan_nvpair(line, &name, &value);
+    if (rc != pcmk_rc_ok) {
+        reason = pcmk_rc_str(rc);
+        goto done;
+    }
+
+    // Leading whitespace is allowed and ignored. A quoted name is invalid.
+    g_strchug(name);
+    if (!valid_env_var_name(name)) {
+        reason = "invalid environment variable name";
+        goto done;
     }
 
-    *first = *last = NULL;
-    return FALSE;
+    /* Parse the value as the shell would do (stripping outermost quotes, etc.).
+     * Also sanity-check that the value either is empty or consists of one
+     * token. Anything malformed should have been caught by now.
+     */
+    if (!g_shell_parse_argv(value, &argc, &argv, &error)) {
+        // Parse error should mean value is empty
+        CRM_CHECK(error->code == G_SHELL_ERROR_EMPTY_STRING, goto done);
+        value_to_set = "";
+
+    } else {
+        // value wasn't empty, so it should contain one token
+        CRM_CHECK(argc == 1, goto done);
+        value_to_set = argv[0];
+    }
+
+    // Don't overwrite (bundle options take precedence)
+    setenv(name, value_to_set, 0);
+
+done:
+    if (reason != NULL) {
+        crm_warn("Failed to perform environment variable assignment '%s': %s",
+                 line, reason);
+    }
+    g_strfreev(argv);
+    g_clear_error(&error);
+    g_free(name);
+    g_free(value);
 }
 
+#define CONTAINER_ENV_FILE "/etc/pacemaker/pcmk-init.env"
+
 static void
-load_env_vars(const char *filename)
+load_env_vars(void)
 {
     /* We haven't forked or initialized logging yet, so don't leave any file
      * descriptors open, and don't log -- silently ignore errors.
      */
-    FILE *fp = fopen(filename, "r");
-
-    if (fp != NULL) {
-        char line[LINE_MAX] = { '\0', };
-
-        while (fgets(line, LINE_MAX, fp) != NULL) {
-            char *name = NULL;
-            char *end = NULL;
-            char *value = NULL;
-            char *quote = NULL;
-
-            // Look for valid name immediately followed by equals sign
-            if (find_env_var_name(line, &name, &end) && (*++end == '=')) {
-
-                // Null-terminate name, and advance beyond equals sign
-                *end++ = '\0';
-
-                // Check whether value is quoted
-                if ((*end == '\'') || (*end == '"')) {
-                    quote = end++;
-                }
-                value = end;
-
-                if (quote) {
-                    /* Value is remaining characters up to next non-backslashed
-                     * matching quote character.
-                     */
-                    while (((*end != *quote) || (*(end - 1) == '\\'))
-                           && (*end != '\0')) {
-                        end++;
-                    }
-                    if (*end == *quote) {
-                        // Null-terminate value, and advance beyond close quote
-                        *end++ = '\0';
-                    } else {
-                        // Matching closing quote wasn't found
-                        value = NULL;
-                    }
-
-                } else {
-                    /* Value is remaining characters up to next non-backslashed
-                     * whitespace.
-                     */
-                    while ((!isspace(*end) || (*(end - 1) == '\\'))
-                           && (*end != '\0')) {
-                        ++end;
-                    }
-
-                    if (end == (line + LINE_MAX - 1)) {
-                        // Line was too long
-                        value = NULL;
-                    }
-                    // Do NOT null-terminate value (yet)
-                }
-
-                /* We have a valid name and value, and end is now the character
-                 * after the closing quote or the first whitespace after the
-                 * unquoted value. Make sure the rest of the line is just
-                 * whitespace or a comment.
-                 */
-                if (value) {
-                    char *value_end = end;
-
-                    while (isspace(*end) && (*end != '\n')) {
-                        ++end;
-                    }
-                    if ((*end == '\n') || (*end == '#')) {
-                        if (quote == NULL) {
-                            // Now we can null-terminate an unquoted value
-                            *value_end = '\0';
-                        }
-
-                        // Don't overwrite (bundle options take precedence)
-                        // coverity[tainted_string] This can't easily be changed right now
-                        setenv(name, value, 0);
-
-                    } else {
-                        value = NULL;
-                    }
-                }
-            }
+    FILE *fp = fopen(CONTAINER_ENV_FILE, "r");
+    char *line = NULL;
+    size_t buf_size = 0;
 
-            if ((value == NULL) && (strchr(line, '\n') == NULL)) {
-                // Eat remainder of line beyond LINE_MAX
-                if (fscanf(fp, "%*[^\n]\n") == EOF) {
-                    value = NULL; // Don't care, make compiler happy
-                }
-            }
-        }
-        fclose(fp);
+    if (fp == NULL) {
+        return;
+    }
+
+    while (getline(&line, &buf_size, fp) != -1) {
+        load_env_var_line(line);
+        errno = 0;
+    }
+
+    // getline() returns -1 on EOF (expected) or error
+    if (errno != 0) {
+        int rc = errno;
+
+        crm_err("Error while reading environment variables from "
+                CONTAINER_ENV_FILE ": %s",
+                pcmk_rc_str(rc));
     }
+    fclose(fp);
+    free(line);
 }
 
 void
@@ -221,7 +238,7 @@ remoted_spawn_pidone(int argc, char **argv)
      * To allow for that, look for a special file containing a shell-like syntax
      * of name/value pairs, and export those into the environment.
      */
-    load_env_vars("/etc/pacemaker/pcmk-init.env");
+    load_env_vars();
 
     if (strcmp(pid1, "vars") == 0) {
         return;

daemons/fenced/fenced_commands.c

@@ -744,10 +744,13 @@ schedule_stonith_command(async_command_t *cmd, fenced_device_t *device)
         delay_base = delay_max;
     }
     if (delay_max > 0) {
-        cmd->start_delay +=
-            // coverity[dont_call] It doesn't matter here if rand() is predictable
-            ((delay_max != delay_base)?(rand() % (delay_max - delay_base)):0)
-            + delay_base;
+        cmd->start_delay += delay_base;
+
+        // Add random offset so that delay_base <= cmd->start_delay <= delay_max
+        if (delay_max > delay_base) {
+            // coverity[dont_call] Doesn't matter that rand() is predictable
+            cmd->start_delay += rand() % (delay_max - delay_base + 1);
+        }
     }
 
     if (cmd->start_delay > 0) {

doc/sphinx/Pacemaker_Explained/collective.rst

@@ -1104,12 +1104,13 @@ greater than zero.
    * Environment variables whose value is the same regardless of the underlying host
      may be set using the container element's ``options`` attribute.
    * If you want variables to have host-specific values, you can use the
-     :ref:`storage-mapping <s-bundle-storage>` element to map a file on the host as
-     ``/etc/pacemaker/pcmk-init.env`` in the container *(since 2.0.3)*.
-     Pacemaker Remote will parse this file as a shell-like format, with
-     variables set as NAME=VALUE, ignoring blank lines and comments starting
-     with "#".
-
+     :ref:`storage-mapping <s-bundle-storage>` element to map a file on the host
+     as |PCMK_INIT_ENV_FILE| in the container *(since 2.0.3)*. Pacemaker Remote
+     will parse this file in a manner similar to a POSIX shell, with variables
+     set as NAME=VALUE, ignoring blank lines and comments starting with ``#``.
+     An assignment may not span multiple lines, and multiple assignments per
+     line are not supported.
+   
 .. important::
 
    When a bundle has a ``primitive``, Pacemaker on all cluster nodes must be able to

lib/common/nvpair.c

@@ -114,8 +114,7 @@ pcmk_free_nvpairs(GSList *nvpairs)
  *
  * \param[in]  input  Input string, likely from the command line
  * \param[out] name   Everything before the first \c '=' in the input string
- * \param[out] value  Everything after the first \c '=' in the input string,
- *                    minus trailing newlines
+ * \param[out] value  Everything after the first \c '=' in the input string
  *
  * \return Standard Pacemaker return code
  *
@@ -125,6 +124,11 @@ pcmk_free_nvpairs(GSList *nvpairs)
 int
 pcmk__scan_nvpair(const gchar *input, gchar **name, gchar **value)
 {
+    /* @COMPAT Consider rejecting leading (and possibly trailing) whitespace in
+     * value and stripping outer quotes from value (for example,
+     * using g_shell_unquote()). This would affect stonith_admin and
+     * crm_resource and would simplify remoted_spawn_pidone()'s helpers.
+     */
     gchar **nvpair = NULL;
     int rc = pcmk_rc_ok;
 
@@ -134,19 +138,15 @@ pcmk__scan_nvpair(const gchar *input, gchar **name, gchar **value)
 
     nvpair = g_strsplit(input, "=", 2);
 
-    /* Check whether nvpair is well-formed (short-circuits if input was split
-     * into fewer than 2 tokens)
-     */
-    if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) {
+    // Check whether nvpair is well-formed: two tokens and non-empty name
+    if ((g_strv_length(nvpair) != 2) || pcmk__str_empty(nvpair[0])) {
         rc = pcmk_rc_bad_nvpair;
         goto done;
     }
 
+    // name and value take ownership of the strings in nvpair
     *name = nvpair[0];
     *value = nvpair[1];
-    pcmk__trim((char *) *value);
-
-    // name and value took ownership
     nvpair[0] = NULL;
     nvpair[1] = NULL;