summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAtin Mukherjee <amukherj@redhat.com>2014-04-08 17:10:25 +0530
committerVijay Bellur <vbellur@redhat.com>2014-05-03 08:20:06 -0700
commite73fc9939aecfa9f7955653d02f12243aba02fc6 (patch)
treef170c95f3b6622aa9fb2937dfef7742ea579a711
parentc6f4504c12d35359986a08da222193057946570a (diff)
glusterd : Volname, brickpath & volfpath length validation
While creating a volume and adding a brick validation for _POSIX_PATH_MAX is done on absolute pathname instead of relative pathname due to which a brickpath having less than _POSIX_PATH_MAX may also fail the validation if the directory length is greater than (_POSIX_PATH_MAX -strlen(brickpath/volume name). Also this fix addresses one cli response message correction which says the volume file is too long instead of brick path is too long (when brickpath length validation doesn't fail and vol file length validation fails.) It is also important to note that with the current design of volfile naming, it can not be guranteed that volname and brickpath can have max of _POSIX_PATH_MAX characters. Change-Id: I1283d1f9dea96ae797620002c8723719f26a866d BUG: 1085330 Signed-off-by: Atin Mukherjee <amukherj@redhat.com> Reviewed-on: http://review.gluster.org/7420 Reviewed-by: Niels de Vos <ndevos@redhat.com> Reviewed-by: Krishnan Parthasarathi <kparthas@redhat.com> Tested-by: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Vijay Bellur <vbellur@redhat.com>
-rw-r--r--cli/src/cli-cmd-parser.c10
-rw-r--r--cli/src/cli-cmd-volume.c2
-rw-r--r--rpc/rpc-lib/src/protocol-common.h2
-rwxr-xr-xtests/bugs/bug-1085330.t80
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-op-sm.h1
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-store.c44
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-utils.c2
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-volgen.c3
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-volume-ops.c13
-rw-r--r--xlators/mgmt/glusterd/src/glusterd.h19
10 files changed, 152 insertions, 24 deletions
diff --git a/cli/src/cli-cmd-parser.c b/cli/src/cli-cmd-parser.c
index e7f41fa..a41c080 100644
--- a/cli/src/cli-cmd-parser.c
+++ b/cli/src/cli-cmd-parser.c
@@ -236,8 +236,11 @@ cli_cmd_volume_create_parse (const char **words, int wordcount, dict_t **options
if (strchr (volname, '/'))
goto out;
- if (strlen (volname) > 512)
+ if (strlen (volname) > GD_VOLUME_NAME_MAX) {
+ cli_err("Volume name exceeds %d characters.",
+ GD_VOLUME_NAME_MAX);
goto out;
+ }
for (i = 0; i < strlen (volname); i++)
if (!isalnum (volname[i]) && (volname[i] != '_') &&
@@ -561,8 +564,11 @@ cli_cmd_quota_parse (const char **words, int wordcount, dict_t **options)
if (strchr (volname, '/'))
goto out;
- if (strlen (volname) > 512)
+ if (strlen (volname) > GD_VOLUME_NAME_MAX) {
+ cli_err("Volname can not exceed %d characters.",
+ GD_VOLUME_NAME_MAX);
goto out;
+ }
for (i = 0; i < strlen (volname); i++)
if (!isalnum (volname[i]) && (volname[i] != '_') &&
diff --git a/cli/src/cli-cmd-volume.c b/cli/src/cli-cmd-volume.c
index 83b923e..6072fcc 100644
--- a/cli/src/cli-cmd-volume.c
+++ b/cli/src/cli-cmd-volume.c
@@ -65,7 +65,7 @@ cli_cmd_volume_info_cbk (struct cli_state *state, struct cli_cmd_word *word,
} else if (wordcount == 3) {
ctx.flags = GF_CLI_GET_VOLUME;
ctx.volname = (char *)words[2];
- if (strlen (ctx.volname) > 1024) {
+ if (strlen (ctx.volname) > GD_VOLUME_NAME_MAX) {
cli_out ("Invalid volume name");
goto out;
}
diff --git a/rpc/rpc-lib/src/protocol-common.h b/rpc/rpc-lib/src/protocol-common.h
index 634fff8..02e0d24 100644
--- a/rpc/rpc-lib/src/protocol-common.h
+++ b/rpc/rpc-lib/src/protocol-common.h
@@ -292,4 +292,6 @@ typedef struct gf_gsync_detailed_status_ gf_gsync_status_t;
#define GD_MGMT_HNDSK_PROGRAM 1239873 /* Completely random */
#define GD_MGMT_HNDSK_VERSION 1
+#define GD_VOLUME_NAME_MAX 256 /* Maximum size of volume name */
+
#endif /* !_PROTOCOL_COMMON_H */
diff --git a/tests/bugs/bug-1085330.t b/tests/bugs/bug-1085330.t
new file mode 100755
index 0000000..dafba21
--- /dev/null
+++ b/tests/bugs/bug-1085330.t
@@ -0,0 +1,80 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+. $(dirname $0)/../volume.rc
+
+STR="1234567890"
+volname="Vol"
+
+cleanup;
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume info;
+
+
+# Construct volname string such that its more than 256 characters
+for i in {1..30}
+do
+ volname+=$STR
+done
+# Now $volname is more than 256 chars
+
+TEST ! $CLI volume create $volname $H0:$B0/${volname}{1,2};
+
+TEST $CLI volume info;
+
+# Construct brick string such that its more than 256 characters
+volname="Vol1234"
+brick="brick"
+for i in {1..30}
+do
+ brick+=$STR
+done
+# Now $brick1 is more than 256 chars
+
+TEST ! $CLI volume create $volname $H0:$B0/$brick;
+
+TEST $CLI volume info;
+
+# Now try to create a volume with couple of bricks (strlen(volname) = 128 &
+# strlen(brick1) = 128
+# Command should still fail as strlen(volp path) > 256
+
+volname="Volume-0"
+brick="brick-00"
+STR="12345678"
+
+for i in {1..15}
+do
+ volname+=$STR
+ brick+=$STR
+done
+TEST ! $CLI volume create $volname $H0:$B0/$brick;
+
+TEST $CLI volume info;
+
+# test case with brick path as 255 and a trailing "/"
+brick=""
+STR1="12345678"
+volname="vol"
+
+for i in {1..31}
+do
+ brick+=$STR1
+done
+brick+="123456/"
+
+echo $brick | wc -c
+# Now $brick is exactly 255 chars, but at end a trailing space
+# This will still fail as volfpath exceeds more than _POSIX_MAX chars
+
+TEST ! $CLI volume create $volname $H0:$B0/$brick;
+
+TEST $CLI volume info;
+
+# Positive test case
+TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2};
+
+TEST $CLI volume info;
+
+cleanup;
diff --git a/xlators/mgmt/glusterd/src/glusterd-op-sm.h b/xlators/mgmt/glusterd/src/glusterd-op-sm.h
index cb67319..229ee46 100644
--- a/xlators/mgmt/glusterd/src/glusterd-op-sm.h
+++ b/xlators/mgmt/glusterd/src/glusterd-op-sm.h
@@ -31,7 +31,6 @@
#include "glusterd.h"
#include "protocol-common.h"
-#define GD_VOLUME_NAME_MAX 256
#define GD_OP_PROTECTED (0x02)
#define GD_OP_UNPROTECTED (0x04)
diff --git a/xlators/mgmt/glusterd/src/glusterd-store.c b/xlators/mgmt/glusterd/src/glusterd-store.c
index 0404e11..26de774 100644
--- a/xlators/mgmt/glusterd/src/glusterd-store.c
+++ b/xlators/mgmt/glusterd/src/glusterd-store.c
@@ -134,12 +134,16 @@ glusterd_store_brickinfopath_set (glusterd_volinfo_t *volinfo,
gf_boolean_t
glusterd_store_is_valid_brickpath (char *volname, char *brick)
{
- char brickpath[PATH_MAX] = {0};
glusterd_brickinfo_t *brickinfo = NULL;
glusterd_volinfo_t *volinfo = NULL;
int32_t ret = 0;
size_t volname_len = strlen (volname);
xlator_t *this = NULL;
+ int bpath_len = 0;
+ const char delim[2] = "/";
+ char *sub_dir = NULL;
+ char *saveptr = NULL;
+ char *brickpath_ptr = NULL;
this = THIS;
GF_ASSERT (this);
@@ -163,10 +167,40 @@ glusterd_store_is_valid_brickpath (char *volname, char *brick)
goto out;
}
memcpy (volinfo->volname, volname, volname_len+1);
- glusterd_store_brickinfopath_set (volinfo, brickinfo, brickpath,
- sizeof (brickpath));
- ret = (strlen (brickpath) < _POSIX_PATH_MAX);
+ /* Check whether brickpath is less than PATH_MAX */
+ ret = 1;
+ bpath_len = strlen (brickinfo->path);
+
+ if (brickinfo->path[bpath_len - 1] != '/') {
+ if (strlen (brickinfo->path) >= PATH_MAX) {
+ ret = 0;
+ goto out;
+ }
+ } else {
+ /* Path has a trailing "/" which should not be considered in
+ * length check validation
+ */
+ if (strlen (brickinfo->path) >= PATH_MAX + 1) {
+ ret = 0;
+ goto out;
+ }
+ }
+
+ /* The following validation checks whether each sub directories in the
+ * brick path meets the POSIX max length validation
+ */
+
+ brickpath_ptr = brickinfo->path;
+ sub_dir = strtok_r (brickpath_ptr, delim, &saveptr);
+
+ while (sub_dir != NULL) {
+ if (strlen(sub_dir) >= _POSIX_PATH_MAX) {
+ ret = 0;
+ goto out;
+ }
+ sub_dir = strtok_r (NULL, delim, &saveptr);
+ }
out:
if (brickinfo)
@@ -2564,7 +2598,7 @@ glusterd_store_retrieve_volume (char *volname, glusterd_snap_t *snap)
if (ret)
goto out;
- strncpy (volinfo->volname, volname, GLUSTERD_MAX_VOLUME_NAME);
+ strncpy (volinfo->volname, volname, GD_VOLUME_NAME_MAX);
volinfo->snapshot = snap;
if (snap)
volinfo->is_snap_volume = _gf_true;
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index b7f81bf..7fd7eae 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -511,7 +511,7 @@ glusterd_volinfo_new (glusterd_volinfo_t **volinfo)
goto out;
}
- snprintf (new_volinfo->parent_volname, GLUSTERD_MAX_VOLUME_NAME, "N/A");
+ snprintf (new_volinfo->parent_volname, GD_VOLUME_NAME_MAX, "N/A");
new_volinfo->snap_max_hard_limit = GLUSTERD_SNAPS_MAX_HARD_LIMIT;
diff --git a/xlators/mgmt/glusterd/src/glusterd-volgen.c b/xlators/mgmt/glusterd/src/glusterd-volgen.c
index 82ff882..a8aa577 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volgen.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volgen.c
@@ -3479,7 +3479,8 @@ glusterd_is_valid_volfpath (char *volname, char *brick)
strncpy (volinfo->volname, volname, sizeof (volinfo->volname));
get_brick_filepath (volfpath, volinfo, brickinfo);
- ret = (strlen (volfpath) < _POSIX_PATH_MAX);
+ ret = ((strlen(volfpath) < PATH_MAX) &&
+ strlen (strrchr(volfpath, '/')) < _POSIX_PATH_MAX);
out:
if (brickinfo)
diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
index 083c7a0..8d126c5 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
@@ -800,14 +800,21 @@ glusterd_op_stage_create_volume (dict_t *dict, char **op_errstr)
brick= strtok_r (brick_list, " \n", &tmpptr);
brick_list = tmpptr;
- if (!glusterd_store_is_valid_brickpath (volname, brick) ||
- !glusterd_is_valid_volfpath (volname, brick)) {
+ if (!glusterd_store_is_valid_brickpath (volname, brick)) {
snprintf (msg, sizeof (msg), "brick path %s is too "
"long.", brick);
ret = -1;
goto out;
}
+ if (!glusterd_is_valid_volfpath (volname, brick)) {
+ snprintf (msg, sizeof (msg), "Volume file path for "
+ "volume %s and brick path %s is too long.",
+ volname, brick);
+ ret = -1;
+ goto out;
+ }
+
ret = glusterd_brickinfo_new_from_brick (brick, &brick_info);
if (ret)
goto out;
@@ -1522,7 +1529,7 @@ glusterd_op_create_volume (dict_t *dict, char **op_errstr)
goto out;
}
- strncpy (volinfo->volname, volname, GLUSTERD_MAX_VOLUME_NAME);
+ strncpy (volinfo->volname, volname, sizeof(volinfo->volname));
GF_ASSERT (volinfo->volname);
ret = dict_get_int32 (dict, "type", &volinfo->type);
diff --git a/xlators/mgmt/glusterd/src/glusterd.h b/xlators/mgmt/glusterd/src/glusterd.h
index b7c0aea..2f63d07 100644
--- a/xlators/mgmt/glusterd/src/glusterd.h
+++ b/xlators/mgmt/glusterd/src/glusterd.h
@@ -39,7 +39,6 @@
#include "syncop.h"
#include "store.h"
-#define GLUSTERD_MAX_VOLUME_NAME 1000
#define GLUSTERD_TR_LOG_SIZE 50
#define GLUSTERD_NAME "glusterd"
#define GLUSTERD_SOCKET_LISTEN_BACKLOG 128
@@ -297,17 +296,17 @@ typedef struct glusterd_replace_brick_ glusterd_replace_brick_t;
struct glusterd_volinfo_ {
gf_lock_t lock;
- char volname[GLUSTERD_MAX_VOLUME_NAME];
gf_boolean_t is_snap_volume;
glusterd_snap_t *snapshot;
uuid_t restored_from_snap;
- char parent_volname[GLUSTERD_MAX_VOLUME_NAME];
+ char parent_volname[GD_VOLUME_NAME_MAX];
/* In case of a snap volume
i.e (is_snap_volume == TRUE) this
field will contain the name of
the volume which is snapped. In
case of a non-snap volume, this
field will be initialized as N/A */
+ char volname[GD_VOLUME_NAME_MAX];
int type;
int brick_count;
uint64_t snap_count;
@@ -521,13 +520,13 @@ typedef ssize_t (*gd_serialize_t) (struct iovec outmsg, void *args);
snprintf (abspath, sizeof (abspath)-1, \
DEFAULT_VAR_RUN_DIRECTORY"/%s%s", volname, path);
-#define GLUSTERD_REMOVE_SLASH_FROM_PATH(path,string) do { \
- int i = 0; \
- for (i = 1; i < strlen (path); i++) { \
- string[i-1] = path[i]; \
- if (string[i-1] == '/') \
- string[i-1] = '-'; \
- } \
+#define GLUSTERD_REMOVE_SLASH_FROM_PATH(path,string) do { \
+ int i = 0; \
+ for (i = 1; i < strlen (path); i++) { \
+ string[i-1] = path[i]; \
+ if (string[i-1] == '/' && (i != strlen(path) - 1)) \
+ string[i-1] = '-'; \
+ } \
} while (0)
#define GLUSTERD_GET_BRICK_PIDFILE(pidfile,volinfo,brickinfo, priv) do { \