Refactor: remoted: Clean up load_env_vars()

This makes two Coverity false positives (tainted name and value passed
to setenv()) go away. It also makes the function's logic easier to
follow.

We now use getline() instead of fgets(). Instead of reading into a
fixed-size buffer, getline() allocates a buffer large enough to hold the
line, and it calls realloc() as needed. This is not an execution path
where performance is critical enough for this to matter.

getline() also returns the number of characters read, so that if there
is a '\0' character within a line, the caller can tell that the end of
line has not yet been reached. This doesn't seem to matter for our use
case, since we don't want to allow null bytes inside a name or value.

We also make liberal use of continue statements to avoid nesting where
possible, and this allows us to get rid of the places where we set value
to NULL.

This will be painful to review, so sorry in advance.

Signed-off-by: Reid Wahl <[email protected]>

Author: nrwahl2

daemons/execd/remoted_pidone.c

@@ -93,103 +93,118 @@ find_env_var_name(char *line, char **first, char **last)
     return FALSE;
 }
 
+#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");
-    char line[LINE_MAX] = { 0, };
+    FILE *fp = fopen(CONTAINER_ENV_FILE, "r");
+    char *line = NULL;
+    size_t buf_size = 0;
 
     if (fp == NULL) {
         return;
     }
 
-    while (fgets(line, LINE_MAX, fp) != NULL) {
+    while (getline(&line, &buf_size, fp) != -1) {
         char *name = NULL;
         char *end = NULL;
-        char *value = NULL;
         char *quote = NULL;
+        char *value = NULL;
+        char *value_end = NULL;
 
         // Look for valid name immediately followed by equals sign
-        if (find_env_var_name(line, &name, &end) && (*++end == '=')) {
+        if (!find_env_var_name(line, &name, &end) || (*++end != '=')) {
+            continue;
+        }
 
-            // Null-terminate name, and advance beyond equals sign
-            *end++ = '\0';
+        // Null-terminate name, and advance beyond equals sign
+        *end++ = '\0';
+
+        // Check whether value is quoted
+        if ((*end == '\'') || (*end == '"')) {
+            quote = end++;
+        }
 
-            // Check whether value is quoted
-            if ((*end == '\'') || (*end == '"')) {
-                quote = end++;
+        // Set beginning of value
+        value = end;
+
+        // Find end of value
+        if (quote != NULL) {
+            /* Value is remaining characters up to next non-backslashed matching
+             * quote character
+             *
+             * @TODO This breaks when the value ends with a literal escaped
+             * backslash. For example: VAR='value\\'.
+             */
+            while (((*end != *quote) || (*(end - 1) == '\\'))
+                   && (*end != '\0')) {
+                end++;
             }
-            value = end;
-
-            if (quote != NULL) {
-                /* 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)
+
+            if (*end != *quote) {
+                // Matching closing quote wasn't found
+                continue;
             }
 
-            /* 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.
+            /* Null-terminate value at the closing quote and advance the end
+             * pointer beyond it.
+             */
+            *end++ = '\0';
+
+        } else {
+            /* Value is remaining characters up to next non-backslashed
+             * whitespace
+             *
+             * @TODO This breaks when the value ends with a literal escaped
+             * backslash and is followed by a space.
              */
-            if (value != NULL) {
-                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)
-                    setenv(name, value, 0);
-
-                } else {
-                    value = NULL;
-                }
+            while ((!isspace(*end) || (*(end - 1) == '\\'))
+                   && (*end != '\0')) {
+                end++;
             }
-        }
 
-        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
+            if (*end == '\0') {
+                // Malformed line (contains null terminator before newline)
+                continue;
             }
         }
+
+        /* 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.
+         */
+        value_end = end;
+
+        while (isspace(*end) && (*end != '\n')) {
+            end++;
+        }
+
+        if ((*end != '\n') && (*end != '#')) {
+            // Found garbage after value
+            continue;
+        }
+
+        // Null-terminate value (it's safe to do it again for a quoted value)
+        *value_end = '\0';
+
+        // Don't overwrite (bundle options take precedence)
+        setenv(name, value, 0);
+    }
+
+    // getline() returns -1 on EOF 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 +236,7 @@ remoted_spawn_pidone(int argc, char **argv, char **envp)
      * 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;