glusterd: fail vol set when value = empty string/string with all whitespaces

PROBLEM:

'volume set' operation accepts empty strings and strings containing
all whitespaces for value. The result - subsequent attempts to start
glusterd fail.

FIX:

Added relevant checks in xlator_option_validate_* functions in options.c
and in conversion functions in common-utils.c to invalidate wrong option
values.

Change-Id: I33232c6b42ab4fcce85c2d0e0b0da145fc9232c3
BUG: 859927
Signed-off-by: Krutika Dhananjay <kdhananj@redhat.com>
Reviewed-on: http://review.gluster.org/4033
Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Anand Avati <avati@redhat.com>

3 files changed, 163 insertions, 38 deletions

diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c
index 40a68795302..c327d4fe43c 100644
--- a/libglusterfs/src/common-utils.c
+++ b/libglusterfs/src/common-utils.c
@@ -666,6 +666,8 @@ gf_string2time (const char *str, uint32_t *n)
         old_errno = errno;
         errno = 0;
         value = strtol (str, &tail, 0);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -709,6 +711,8 @@ gf_string2percent (const char *str, double *n)
         old_errno = errno;
         errno = 0;
         value = strtod (str, &tail);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -742,6 +746,8 @@ _gf_string2long (const char *str, long *n, int base)
         old_errno = errno;
         errno = 0;
         value = strtol (str, &tail, base);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -782,6 +788,8 @@ _gf_string2ulong (const char *str, unsigned long *n, int base)
         old_errno = errno;
         errno = 0;
         value = strtoul (str, &tail, base);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -822,6 +830,8 @@ _gf_string2uint (const char *str, unsigned int *n, int base)
         old_errno = errno;
         errno = 0;
         value = strtoul (str, &tail, base);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -853,6 +863,8 @@ _gf_string2double (const char *str, double *n)
         old_errno = errno;
         errno = 0;
         value = strtod (str, &tail);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -884,6 +896,8 @@ _gf_string2longlong (const char *str, long long *n, int base)
         old_errno = errno;
         errno = 0;
         value = strtoll (str, &tail, base);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -924,6 +938,8 @@ _gf_string2ulonglong (const char *str, unsigned long long *n, int base)
         old_errno = errno;
         errno = 0;
         value = strtoull (str, &tail, base);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -1288,6 +1304,8 @@ gf_string2bytesize (const char *str, uint64_t *n)
         old_errno = errno;
         errno = 0;
         value = strtod (str, &tail);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
@@ -1344,6 +1362,8 @@ gf_string2percent_or_bytesize (const char *str,
         old_errno = errno;
         errno = 0;
         value = strtoull (str, &tail, 10);
+        if (str == tail)
+                errno = EINVAL;
 
         if (errno == ERANGE || errno == EINVAL)
                 return -1;
diff --git a/libglusterfs/src/options.c b/libglusterfs/src/options.c
index f8ebb94c6d7..5d436bab38d 100644
--- a/libglusterfs/src/options.c
+++ b/libglusterfs/src/options.c
@@ -228,6 +228,46 @@ out:
         return ret;
 }
 
+void
+set_error_str (char *errstr, size_t len, volume_option_t *opt, const char *key,
+               const char *value)
+{
+        int i = 0;
+        char given_array[4096] = {0,};
+
+        for (i = 0; (i < ZR_OPTION_MAX_ARRAY_SIZE) && opt->value[i];) {
+                strcat (given_array, opt->value[i]);
+                if (((++i) < ZR_OPTION_MAX_ARRAY_SIZE) &&
+                    (opt->value[i]))
+                        strcat (given_array, ", ");
+                else
+                        strcat (given_array, ".");
+        }
+        snprintf (errstr, len, "option %s %s: '%s' is not valid "
+                  "(possible options are %s)", key, value, value, given_array);
+        return;
+}
+
+int
+is_all_whitespaces (const char *value)
+{
+        int i = 0;
+        size_t len = 0;
+
+        if (value == NULL)
+                return -1;
+
+        len = strlen (value);
+
+        for (i = 0; i < len; i++) {
+                if (value[i] == ' ')
+                        continue;
+                else
+                        return 0;
+        }
+
+        return 1;
+}
 
 static int
 xlator_option_validate_str (xlator_t *xl, const char *key, const char *value,
@@ -235,8 +275,7 @@ xlator_option_validate_str (xlator_t *xl, const char *key, const char *value,
 {
         int          ret = -1;
         int          i = 0;
-        char         errstr[256];
-        char         given_array[4096] = {0,};
+        char         errstr[4096] = {0,};
 
         /* Check if the '*str' is valid */
         if (GF_OPTION_LIST_EMPTY(opt)) {
@@ -244,6 +283,9 @@ xlator_option_validate_str (xlator_t *xl, const char *key, const char *value,
                 goto out;
         }
 
+        if (is_all_whitespaces (value) == 1)
+                goto out;
+
         for (i = 0; (i < ZR_OPTION_MAX_ARRAY_SIZE) && opt->value[i]; i++) {
  #ifdef  GF_DARWIN_HOST_OS
                 if (fnmatch (opt->value[i], value, 0) == 0) {
@@ -258,8 +300,8 @@ xlator_option_validate_str (xlator_t *xl, const char *key, const char *value,
  #endif
         }
 
-        if (((i < ZR_OPTION_MAX_ARRAY_SIZE) && (!opt->value[i])) ||
-            (i == ZR_OPTION_MAX_ARRAY_SIZE)) {
+        if ((i == ZR_OPTION_MAX_ARRAY_SIZE) || (!opt->value[i]))
+                goto out;
                 /* enter here only if
                  * 1. reached end of opt->value array and haven't
                  *    validated input
@@ -269,26 +311,15 @@ xlator_option_validate_str (xlator_t *xl, const char *key, const char *value,
                  *    matched all possible input values.
                  */
 
-                for (i = 0; (i < ZR_OPTION_MAX_ARRAY_SIZE) && opt->value[i];) {
-                        strcat (given_array, opt->value[i]);
-                        if (((++i) < ZR_OPTION_MAX_ARRAY_SIZE) &&
-                           (opt->value[i]))
-                                strcat (given_array, ", ");
-                        else
-                                strcat (given_array, ".");
-                }
-                snprintf (errstr, 256,
-                          "option %s %s: '%s' is not valid "
-                          "(possible options are %s)",
-                          key, value, value, given_array);
-                gf_log (xl->name, GF_LOG_ERROR, "%s", errstr);
-                goto out;
-        }
-
         ret = 0;
+
 out:
-        if (ret && op_errstr)
-                *op_errstr = gf_strdup (errstr);
+        if (ret) {
+                set_error_str (errstr, sizeof (errstr), opt, key, value);
+                gf_log (xl->name, GF_LOG_ERROR, "%s", errstr);
+                if (op_errstr)
+                        *op_errstr = gf_strdup (errstr);
+        }
         return ret;
 }
 
@@ -513,32 +544,31 @@ xlator_option_validate_addr_list (xlator_t *xl, const char *key,
         char         *dup_val = NULL;
         char         *addr_tok = NULL;
         char         *save_ptr = NULL;
-        char         errstr[256];
+        char         errstr[4096] = {0,};
 
         dup_val = gf_strdup (value);
-        if (!dup_val) {
-                ret = -1;
-                snprintf (errstr, 256, "internal error, out of memory.");
+        if (!dup_val)
                 goto out;
-        }
 
         addr_tok = strtok_r (dup_val, ",", &save_ptr);
+        if (addr_tok == NULL)
+                goto out;
         while (addr_tok) {
-                if (!valid_internet_address (addr_tok, _gf_true)) {
-                        snprintf (errstr, 256,
-                                  "option %s %s: '%s' is not a valid "
-                                  "internet-address-list",
-                                  key, value, value);
-                        gf_log (xl->name, GF_LOG_ERROR, "%s", errstr);
-                        ret = -1;
+                if (!valid_internet_address (addr_tok, _gf_true))
                         goto out;
-                }
+
                 addr_tok = strtok_r (NULL, ",", &save_ptr);
         }
         ret = 0;
- out:
-        if (op_errstr && ret)
-                *op_errstr = gf_strdup (errstr);
+
+out:
+        if (ret) {
+                snprintf (errstr, sizeof (errstr), "option %s %s: '%s' is not "
+                "a valid internet-address-list", key, value, value);
+                gf_log (xl->name, GF_LOG_ERROR, "%s", errstr);
+                if (op_errstr)
+                        *op_errstr = gf_strdup (errstr);
+        }
         GF_FREE (dup_val);
 
         return ret;
@@ -595,6 +625,10 @@ validate_list_elements (const char *string, volume_option_t *opt,
                 goto out;
 
         str_ptr = strtok_r (dup_string, ",", &str_sav);
+        if (str_ptr == NULL) {
+                ret = -1;
+                goto out;
+        }
         while (str_ptr) {
 
                 key = strtok_r (str_ptr, ":", &substr_sav);
@@ -620,6 +654,7 @@ validate_list_elements (const char *string, volume_option_t *opt,
                 str_ptr = strtok_r (NULL, ",", &str_sav);
                 substr_sav = NULL;
         }
+
  out:
         GF_FREE (dup_string);
         gf_log (THIS->name, GF_LOG_DEBUG, "Returning %d", ret);
diff --git a/tests/bugs/bug-859927.t b/tests/bugs/bug-859927.t
new file mode 100755
index 00000000000..ed74d3eb831
--- /dev/null
+++ b/tests/bugs/bug-859927.t
@@ -0,0 +1,70 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+. $(dirname $0)/../volume.rc
+cleanup;
+
+glusterd;
+
+TEST $CLI volume create $V0 replica 2 stripe 2 $H0:$B0/${V0}{1,2,3,4,5,6,7,8};
+
+TEST ! $CLI volume set $V0 statedump-path ""
+TEST ! $CLI volume set $V0 statedump-path "     "
+TEST   $CLI volume set $V0 statedump-path "/home/"
+EXPECT "/home/" volume_option $V0 server.statedump-path
+
+TEST ! $CLI volume set $V0 background-self-heal-count ""
+TEST ! $CLI volume set $V0 background-self-heal-count "      "
+TEST   $CLI volume set $V0 background-self-heal-count 10
+EXPECT "10" volume_option $V0 cluster.background-self-heal-count
+
+TEST ! $CLI volume set $V0 cache-size ""
+TEST ! $CLI volume set $V0 cache-size "    "
+TEST   $CLI volume set $V0 cache-size 512MB
+EXPECT "512MB" volume_option $V0 performance.cache-size
+
+TEST ! $CLI volume set $V0 self-heal-daemon ""
+TEST ! $CLI volume set $V0 self-heal-daemon "    "
+TEST   $CLI volume set $V0 self-heal-daemon on
+EXPECT "on" volume_option $V0 cluster.self-heal-daemon
+
+TEST ! $CLI volume set $V0 read-subvolume ""
+TEST ! $CLI volume set $V0 read-subvolume "    "
+TEST   $CLI volume set $V0 read-subvolume $V0-client-0
+EXPECT "$V0-client-0" volume_option  $V0 cluster.read-subvolume
+
+TEST ! $CLI volume set $V0 data-self-heal-algorithm ""
+TEST ! $CLI volume set $V0 data-self-heal-algorithm "     "
+TEST ! $CLI volume set $V0 data-self-heal-algorithm on
+TEST   $CLI volume set $V0 data-self-heal-algorithm full
+EXPECT "full" volume_option $V0 cluster.data-self-heal-algorithm
+
+TEST ! $CLI volume set $V0 min-free-inodes ""
+TEST ! $CLI volume set $V0 min-free-inodes "     "
+TEST   $CLI volume set $V0 min-free-inodes 60%
+EXPECT "60%" volume_option $V0 cluster.min-free-inodes
+
+TEST ! $CLI volume set $V0 min-free-disk ""
+TEST ! $CLI volume set $V0 min-free-disk "     "
+TEST   $CLI volume set $V0 min-free-disk 60%
+EXPECT "60%" volume_option $V0 cluster.min-free-disk
+
+TEST   $CLI volume set $V0 min-free-disk 120
+EXPECT "120" volume_option $V0 cluster.min-free-disk
+
+TEST ! $CLI volume set $V0 frame-timeout ""
+TEST ! $CLI volume set $V0 frame-timeout "      "
+TEST   $CLI volume set $V0 frame-timeout 0
+EXPECT "0" volume_option $V0 network.frame-timeout
+
+TEST ! $CLI volume set $V0 auth.allow ""
+TEST ! $CLI volume set $V0 auth.allow "       "
+TEST   $CLI volume set $V0 auth.allow 192.168.122.1
+EXPECT "192.168.122.1" volume_option $V0 auth.allow
+
+TEST ! $CLI volume set $V0 stripe-block-size ""
+TEST ! $CLI volume set $V0 stripe-block-size "      "
+TEST   $CLI volume set $V0 stripe-block-size 512MB
+EXPECT "512MB" volume_option $V0 cluster.stripe-block-size
+
+cleanup;