MXS-1847: Fix server_get_parameter

The function now takes an output buffer as a parameter. This prevents race
conditions by copying the parameter value into a local buffer.

include/maxscale/server.h

@@ -348,7 +348,7 @@ extern void server_set_status_nolock(SERVER *server, uint64_t bit);
 extern void server_clear_status_nolock(SERVER *server, uint64_t bit);
 extern void server_transfer_status(SERVER *dest_server, const SERVER *source_server);
 extern void server_add_mon_user(SERVER *server, const char *user, const char *passwd);
-extern const char *server_get_parameter(const SERVER *server, const char *name);
+extern bool server_get_parameter(const SERVER *server, const char *name, char* out, size_t size);
 extern void server_update_credentials(SERVER *server, const char *user, const char *passwd);
 extern DCB* server_get_persistent(SERVER *server, const char *user, const char* ip, const char *protocol, int id);
 extern void server_update_address(SERVER *server, const char *address);

server/core/server.cc

@@ -924,24 +924,35 @@ static void server_parameter_free(SERVER_PARAM *tofree)
 /**
  * Retrieve a parameter value from a server
  *
- * @param server        The server we are looking for a parameter of
+ * @param server The server we are looking for a parameter of
- * @param name          The name of the parameter we require
+ * @param name   The name of the parameter we require
- * @return      The parameter value or NULL if not found
+ * @param out    Buffer where value is stored, use NULL to check if the parameter exists
+ * @param size   Size of @c out, ignored if @c out is NULL
+ *
+ * @return True if parameter was found
  */
-const char *
+bool server_get_parameter(const SERVER *server, const char *name, char* out, size_t size)
-server_get_parameter(const SERVER *server, const char *name)
 {
+    bool found = false;
     SERVER_PARAM *param = server->parameters;
+    spinlock_acquire(&server->lock);
 
     while (param)
     {
         if (strcmp(param->name, name) == 0 && param->active)
         {
-            return param->value;
+            if (out)
+            {
+                snprintf(out, size, "%s", param->value);
+            }
+            found = true;
+            break;
         }
         param = param->next;
     }
-    return NULL;
+
+    spinlock_release(&server->lock);
+    return found;
 }
 
 /**

server/core/service.cc

@@ -2253,6 +2253,7 @@ static void service_calculate_weights(SERVICE *service)
 
     if (*weightby && service->dbref)
     {
+        char buf[50]; // Enough to hold most numbers
         /** Service has a weighting parameter and at least one server */
         int total = 0;
 
@@ -2260,10 +2261,10 @@ static void service_calculate_weights(SERVICE *service)
         for (SERVER_REF *server = service->dbref; server; server = server->next)
         {
             server->weight = SERVICE_BASE_SERVER_WEIGHT;
-            const char *param = server_get_parameter(server->server, weightby);
+
-            if (param)
+            if (server_get_parameter(server->server, weightby, buf, sizeof(buf)))
             {
-                total += atoi(param);
+                total += atoi(buf);
             }
         }
 
@@ -2284,10 +2285,9 @@ static void service_calculate_weights(SERVICE *service)
             /** Calculate the relative weight of the servers */
             for (SERVER_REF *server = service->dbref; server; server = server->next)
             {
-                const char *param = server_get_parameter(server->server, weightby);
+                if (server_get_parameter(server->server, weightby, buf, sizeof(buf)))
-                if (param)
                 {
-                    int wght = atoi(param);
+                    int wght = atoi(buf);
                     int perc = (wght * SERVICE_BASE_SERVER_WEIGHT) / total;
 
                     if (perc == 0)

server/core/test/test_server.cc

@@ -63,12 +63,13 @@ test1()
     //ss_info_dassert(NULL != service, "New server with valid protocol and port must not be null");
     //ss_info_dassert(0 != service_isvalid(service), "Service must be valid after creation");
 
+    char buf[120];
     ss_dfprintf(stderr, "\t..done\nTest Parameter for Server.");
-    ss_info_dassert(NULL == server_get_parameter(server, (char*)"name"), "Parameter should be null when not set");
+    ss_info_dassert(!server_get_parameter(server, "name", buf, sizeof(buf)), "Parameter should be null when not set");
     server_add_parameter(server, "name", "value");
     mxs_log_flush_sync();
-    ss_info_dassert(0 == strcmp("value", server_get_parameter(server, (char*)"name")),
+    ss_dassert(server_get_parameter(server, "name", buf, sizeof(buf)));
-                    "Parameter should be returned correctly");
+    ss_info_dassert(strcmp("value", buf) == 0, "Parameter should be returned correctly");
     ss_dfprintf(stderr, "\t..done\nTesting Unique Name for Server.");
     ss_info_dassert(NULL == server_find_by_unique_name("non-existent"),
                     "Should not find non-existent unique name.");

server/modules/monitor/galeramon/galeramon.c

@@ -687,7 +687,6 @@ static MXS_MONITORED_SERVER *get_candidate_master(MXS_MONITOR* mon)
     long min_id = -1;
     int minval = INT_MAX;
     int currval;
-    const char* value;
     /* set min_id to the lowest value of moitor_servers->server->node_id */
     while (moitor_servers)
     {
@@ -695,11 +694,11 @@ static MXS_MONITORED_SERVER *get_candidate_master(MXS_MONITOR* mon)
         {
 
             moitor_servers->server->depth = 0;
-
+            char buf[50]; // Enough to hold most numbers
-            if (handle->use_priority && (value = server_get_parameter(moitor_servers->server, "priority")) != NULL)
+            if (handle->use_priority && server_get_parameter(moitor_servers->server, "priority", buf, sizeof(buf)))
             {
                 /** The server has a priority  */
-                if ((currval = atoi(value)) > 0)
+                if ((currval = atoi(buf)) > 0)
                 {
                     /** The priority is valid */
                     if (currval < minval && currval > 0)
@@ -712,7 +711,8 @@ static MXS_MONITORED_SERVER *get_candidate_master(MXS_MONITOR* mon)
             else if (moitor_servers->server->node_id >= 0 &&
                      (!handle->use_priority || /** Server priority disabled*/
                       candidate_master == NULL || /** No candidate chosen */
-                      server_get_parameter(candidate_master->server, "priority") == NULL)) /** Candidate has no priority */
+                       /** Candidate has no priority */
+                      !server_get_parameter(moitor_servers->server, "priority", buf, sizeof(buf))))
             {
                 if (min_id < 0 || moitor_servers->server->node_id < min_id)
                 {
@@ -846,8 +846,9 @@ static void update_sst_donor_nodes(MXS_MONITOR *mon, int is_cluster)
              * If no server has "priority" set, then
              * the server list will be order by default method.
              */
+
             if (handle->use_priority &&
-                server_get_parameter(ptr->server, "priority"))
+                server_get_parameter(ptr->server, "priority", NULL, 0))
             {
                 ignore_priority = false;
             }
@@ -980,28 +981,29 @@ static int compare_node_priority (const void *a, const void *b)
 {
     const MXS_MONITORED_SERVER *s_a = *(MXS_MONITORED_SERVER * const *)a;
     const MXS_MONITORED_SERVER *s_b = *(MXS_MONITORED_SERVER * const *)b;
-
+    char pri_a[50];
-    const char *pri_a = server_get_parameter(s_a->server, "priority");
+    char pri_b[50];
-    const char *pri_b = server_get_parameter(s_b->server, "priority");
+    bool have_a = server_get_parameter(s_a->server, "priority", pri_a, sizeof(pri_a));
+    bool have_b = server_get_parameter(s_b->server, "priority", pri_b, sizeof(pri_b));
 
     /**
      * Check priority parameter:
      *
      * Return a - b in case of issues
      */
-    if (!pri_a && pri_b)
+    if (!have_a && have_b)
     {
         MXS_DEBUG("Server %s has no given priority. It will be at the beginning of the list",
                   s_a->server->unique_name);
         return -(INT_MAX - 1);
     }
-    else if (pri_a && !pri_b)
+    else if (have_a && !have_b)
     {
         MXS_DEBUG("Server %s has no given priority. It will be at the beginning of the list",
                   s_b->server->unique_name);
         return INT_MAX - 1;
     }
-    else if (!pri_a && !pri_b)
+    else if (!have_a && !have_b)
     {
         MXS_DEBUG("Servers %s and %s have no given priority. They be at the beginning of the list",
                   s_a->server->unique_name,