Refactor: remoted: Clean up load_env_vars()

The changes in this commit involve using (gchar *) strings and some
dynamic allocation to make load_env_vars() easier to reason about.

Previously, we tried to be as efficient as possible by reading a line
into a buffer, iterating over it with multiple pointers, dividing
it into name and value by insertion of a null terminator, etc.

This is premature optimization. This function is not called along a path
of execution where performance is so critical as to require this. In my
opinion, clarity is more important.

* Use pcmk__scan_nvpair() to separate the line on the equal sign.
* Simplify name validation since we now have name as a separate string
  (before the equal sign) and don't need to return first and last
  pointers.
* Use an is_quoted boolean variable, so that we can limit the quote
  character to the inner block and compare to the value instead of
  maintaining a pointer to the quote character.
* Skip the leading quote by converting it to a space and calling
  g_strchug() to memmove() value up by one position. This ensures that
  value still points to the beginning of the buffer, which simplifies
  freeing it later.

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

Author: nrwahl2

daemons/execd/remoted_pidone.c

@@ -61,32 +61,26 @@ 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)
 {
-    *first = line;
-
-    if (isalpha(**first) || (**first == '_')) { // Valid first character
-        *last = *first;
-        while (isalnum(*(*last + 1)) || (*(*last + 1) == '_')) {
-            ++*last;
-        }
-        return TRUE;
+    if (!isalpha(*name) && (*name != '_')) {
+        // Invalid first character
+        return false;
     }
 
-    *first = *last = NULL;
-    return FALSE;
+    // The rest of the characters must be alphanumeric or underscores
+    for (name++; isalnum(*name) || (*name == '_'); name++);
+    return *name == '\0';
 }
 
 #define CONTAINER_ENV_FILE "/etc/pacemaker/pcmk-init.env"
@@ -106,50 +100,56 @@ load_env_vars(void)
     }
 
     while (getline(&line, &buf_size, fp) != -1) {
-        char *name = NULL;
-        char *end = NULL;
-        char *value = NULL;
-        char *comment = NULL;
+        gchar *name = NULL;
+        gchar *value = NULL;
+        gchar *end = NULL;
+        gchar *comment = NULL;
+        bool is_quoted = false;
 
         // Strip leading and trailing whitespace
         g_strstrip(line);
 
-        // Look for valid name immediately followed by equals sign
-        if (!find_env_var_name(line, &name, &end) || (*++end != '=')) {
+        if (pcmk__scan_nvpair(line, &name, &value) != pcmk_rc_ok) {
+            crm_warn("Ignoring malformed line while reading environment "
+                     "variables from " CONTAINER_ENV_FILE ": '%s'",
+                     line);
             goto cleanup_loop;
         }
 
-        // Null-terminate name, and advance beyond equals sign
-        *end++ = '\0';
+        if (!valid_env_var_name(name)) {
+            goto cleanup_loop;
+        }
 
-        // Check whether value is quoted
-        if ((*end == '\'') || (*end == '"')) {
-            const char *quote = *end++;
+        is_quoted = (*value == '\'') || (*value == '"');
+        if (is_quoted) {
+            char quote = *value;
 
-            value = end;
+            // Strip the leading quote
+            *value = ' ';
+            g_strchug(value);
 
             /* Value is remaining characters up to next non-backslashed matching
              * quote character.
              */
-            while (((*end != *quote) || (*(end - 1) == '\\'))
-                   && (*end != '\0')) {
-                end++;
-            }
-            if (*end != *quote) {
+            for (end = value;
+                 (*end != '\0') && ((*end != quote) || (*(end - 1) == '\\'));
+                 end++);
+
+            if (*end != quote) {
                 // Matching closing quote wasn't found
                 goto cleanup_loop;
             }
+
             // Discard closing quote and advance to check for trailing garbage
             *end++ = '\0';
 
         } else {
             /* Value is remaining characters up to next non-backslashed
              * whitespace.
              */
-            while ((!isspace(*end) || (*(end - 1) == '\\'))
-                   && (*end != '\0')) {
-                end++;
-            }
+            for (end = value;
+                 (*end != '\0') && (!isspace(*end) || (*(end - 1) == '\\'));
+                 end++);
         }
 
         /* We have a valid name and value, and end is now the character after
@@ -176,6 +176,8 @@ load_env_vars(void)
         setenv(name, value, 0);
 
 cleanup_loop:
+        g_free(name);
+        g_free(value);
         errno = 0;
     }