diff options
author | Nithya Balachandran <nbalacha@redhat.com> | 2014-12-17 13:58:56 +0530 |
---|---|---|
committer | Raghavendra Bhat <raghavendra@redhat.com> | 2015-02-18 22:55:08 -0800 |
commit | f1c4ce0e220a46b7a43c9303c0d137498d421101 (patch) | |
tree | c710507701ad74105e74d0345a4349cd4b9ea740 | |
parent | 692dd7e83cc8a1c85581dda3f752d5ea3b184f8c (diff) |
Storage/posix : Adding error checks in path formation
Renaming directories can cause the size of the buffer
required for posix_handle_path to increase between the
first call, which calculates the size, and the second call
which forms the path in the buffer allocated based on
the size calculated in the first call.
The path created in the second call overflows the
allocated buffer and overwrites the stack causing the
brick process to crash.
The fix adds a buffer size check to prevent the buffer
overflow. It also checks and returns an error if the
posix_handle_path call is unable to form the path instead
of working on the incomplete path, which is likely to cause
subsequent calls using the path to fail with ELOOP.
Preventing buffer overflow and handling errors
BUG: 1113960
Change-Id: If3d3c1952e297ad14f121f05f90a35baf42923aa
Signed-off-by: Nithya Balachandran <nbalacha@redhat.com>
Reviewed-on: http://review.gluster.org/9289
Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Raghavendra Bhat <raghavendra@redhat.com>
-rw-r--r-- | libglusterfs/src/iatt.h | 1 | ||||
-rwxr-xr-x | tests/bugs/posix/bug-1113960.t | 98 | ||||
-rw-r--r-- | xlators/cluster/dht/src/dht-common.c | 6 | ||||
-rw-r--r-- | xlators/storage/posix/src/posix-handle.c | 23 | ||||
-rw-r--r-- | xlators/storage/posix/src/posix-handle.h | 15 | ||||
-rw-r--r-- | xlators/storage/posix/src/posix-helpers.c | 33 | ||||
-rw-r--r-- | xlators/storage/posix/src/posix.c | 133 |
7 files changed, 289 insertions, 20 deletions
diff --git a/libglusterfs/src/iatt.h b/libglusterfs/src/iatt.h index 60ae590..da6b83d 100644 --- a/libglusterfs/src/iatt.h +++ b/libglusterfs/src/iatt.h @@ -77,6 +77,7 @@ struct iatt { #define IA_ISCHR(t) (t == IA_IFCHR) #define IA_ISFIFO(t) (t == IA_IFIFO) #define IA_ISSOCK(t) (t == IA_IFSOCK) +#define IA_ISINVAL(t) (t == IA_INVAL) #define IA_PROT_RUSR(prot) ((prot).owner.read == 1) #define IA_PROT_WUSR(prot) ((prot).owner.write == 1) diff --git a/tests/bugs/posix/bug-1113960.t b/tests/bugs/posix/bug-1113960.t new file mode 100755 index 0000000..ee42de2 --- /dev/null +++ b/tests/bugs/posix/bug-1113960.t @@ -0,0 +1,98 @@ +#!/bin/bash + +. $(dirname $0)/../../include.rc +. $(dirname $0)/../../volume.rc +. $(dirname $0)/../../nfs.rc + +NUM_DIRS=50 +NUM_FILES=10 + +create_dirs () { + for (( i=1; i<=$NUM_DIRS; i+=1));do + mkdir $1/olddir$i + for (( j=0; j<$NUM_FILES; j+=1));do + echo "This is file $j in dir $i" > $1/olddir$i/file$j; + done + done + echo "0" > $M0/status +} + +move_dirs () { + old_path="$1" + new_path="$1" + + #Create a deep directory + for (( i=$NUM_DIRS; i>=2; i-=1));do + mv $1/olddir$i $1/olddir`expr $i - 1` > /dev/null 2>&1; + done + + #Start renaming files and dirs so the paths change for the + #posix_handle_path calculations + + for (( i=1; i<=$NUM_DIRS; i+=1));do + old_path="$new_path/olddir$i" + new_path="$new_path/longernamedir$i" + mv $old_path $new_path; + + for (( j=0; j<$NUM_FILES; j+=1));do + mv $new_path/file$j $new_path/newfile$j > /dev/null 2>&1; + done + done + echo "done" > $M0/status +} + +ls_loop () { + #Loop until the move_dirs function is done + for (( i=0; i<=500; i+=1 )); do + ls -lR $1 > /dev/null 2>&1 + done +} + + +cleanup; + +TEST glusterd +TEST pidof glusterd +TEST $CLI volume info; + +TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2,3,4}; + +EXPECT "$V0" volinfo_field $V0 'Volume Name'; +EXPECT 'Created' volinfo_field $V0 'Status'; +EXPECT '4' brick_count $V0 + +TEST $CLI volume start $V0; +EXPECT 'Started' volinfo_field $V0 'Status'; + +## Mount FUSE with caching disabled (read-write) +TEST glusterfs --entry-timeout=0 --attribute-timeout=0 -s $H0 --volfile-id $V0 $M0; + +TEST create_dirs $M0 + +## Mount FUSE with caching disabled (read-write) again +TEST glusterfs --entry-timeout=0 --attribute-timeout=0 -s $H0 --volfile-id $V0 $M1; + +TEST glusterfs -s $H0 --volfile-id $V0 $M2; + +(ls_loop $M1)& +ls_pid1=$! + +(ls_loop $M2)& +ls_pid2=$! + + +#Start moving/renaming the directories so the paths change +TEST move_dirs $M0 + +EXPECT_WITHIN 180 "done" cat $M0/status + +#Kill the ls processes + +kill -SIGTERM $ls_pid1 +kill -SIGTERM $ls_pid2 + + +#Check if all bricks are still up +EXPECT '4' online_brick_count $V0 + +cleanup; diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c index bc9d04d..3c15996 100644 --- a/xlators/cluster/dht/src/dht-common.c +++ b/xlators/cluster/dht/src/dht-common.c @@ -3789,6 +3789,12 @@ dht_readdirp_cbk (call_frame_t *frame, void *cookie, xlator_t *this, int op_ret, list_for_each_entry (orig_entry, (&orig_entries->list), list) { next_offset = orig_entry->d_off; + + if (IA_ISINVAL(orig_entry->d_stat.ia_type)) { + /*stat failed somewhere- ignore this entry*/ + continue; + } + if (check_is_dir (NULL, (&orig_entry->d_stat), NULL)) { /*Directory entries filtering : diff --git a/xlators/storage/posix/src/posix-handle.c b/xlators/storage/posix/src/posix-handle.c index 7ab6543..5d184d9 100644 --- a/xlators/storage/posix/src/posix-handle.c +++ b/xlators/storage/posix/src/posix-handle.c @@ -296,8 +296,12 @@ posix_handle_pump (xlator_t *this, char *buf, int len, int maxlen, blen = link_len - 48; - if(len + blen >= maxlen) + if (len + blen >= maxlen) { + gf_log (this->name, GF_LOG_ERROR, + "Unable to form handle path for %s (maxlen = %d)", + buf, maxlen); goto err; + } memmove (buf + base_len + blen, buf + base_len, (strlen (buf) - base_len) + 1); @@ -371,11 +375,11 @@ posix_handle_path (xlator_t *this, uuid_t gfid, const char *basename, errno = 0; ret = posix_handle_pump (this, buf, len, maxlen, base_str, base_len, pfx_len); + len = ret; + if (ret == -1) break; - len = ret; - ret = lstat (buf, &stat); } while ((ret == -1) && errno == ELOOP); @@ -841,13 +845,18 @@ posix_handle_unset (xlator_t *this, uuid_t gfid, const char *basename) struct iatt stat; char *path = NULL; - if (!basename) { ret = posix_handle_unset_gfid (this, gfid); return ret; } MAKE_HANDLE_PATH (path, this, gfid, basename); + if (!path) { + gf_log (this->name, GF_LOG_WARNING, + "Failed to create handle path for %s (%s)", + basename, uuid_utoa(gfid)); + return -1; + } ret = posix_istat (this, gfid, basename, &stat); @@ -872,6 +881,12 @@ posix_create_link_if_gfid_exists (xlator_t *this, uuid_t gfid, char *newpath = NULL; MAKE_HANDLE_PATH (newpath, this, gfid, NULL); + if (!newpath) { + gf_log (this->name, GF_LOG_WARNING, + "Failed to create handle path (%s)", uuid_utoa(gfid)); + return ret; + } + ret = lstat (newpath, &stbuf); if (!ret) { ret = sys_link (newpath, real_path); diff --git a/xlators/storage/posix/src/posix-handle.h b/xlators/storage/posix/src/posix-handle.h index 0ce9575..3af1a70 100644 --- a/xlators/storage/posix/src/posix-handle.h +++ b/xlators/storage/posix/src/posix-handle.h @@ -146,6 +146,8 @@ break; \ var = alloca (__len); \ __len = posix_handle_path (this, gfid, base, var, __len); \ + if (__len <= 0) \ + var = NULL; \ } while (0) @@ -192,7 +194,13 @@ errno = 0; \ op_ret = posix_istat (this, loc->gfid, NULL, iatt_p); \ if (errno != ELOOP) { \ - MAKE_HANDLE_PATH (rpath, this, loc->gfid, NULL); \ + MAKE_HANDLE_PATH (rpath, this, (loc)->gfid, NULL); \ + if (!rpath) { \ + op_ret = -1; \ + gf_log (this->name, GF_LOG_ERROR, \ + "Failed to create inode handle " \ + "for path %s", (loc)->path); \ + } \ break; \ } \ /* __ret == -1 && errno == ELOOP */ \ @@ -220,6 +228,11 @@ if (errno != ELOOP) { \ MAKE_HANDLE_PATH (parp, this, loc->pargfid, NULL); \ MAKE_HANDLE_PATH (entp, this, loc->pargfid, loc->name); \ + if (!parp || !entp) { \ + gf_log (this->name, GF_LOG_ERROR, \ + "Failed to create entry handle " \ + "for path %s", loc->path); \ + } \ break; \ } \ /* __ret == -1 && errno == ELOOP */ \ diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c index 62b0d2e..2920f31 100644 --- a/xlators/storage/posix/src/posix-helpers.c +++ b/xlators/storage/posix/src/posix-helpers.c @@ -465,10 +465,17 @@ posix_istat (xlator_t *this, uuid_t gfid, const char *basename, int ret = 0; struct posix_private *priv = NULL; - priv = this->private; MAKE_HANDLE_PATH (real_path, this, gfid, basename); + if (!real_path) { + gf_log (this->name, GF_LOG_ERROR, + "Failed to create handle path for %s/%s", + uuid_utoa (gfid), basename ? basename : ""); + errno = ESTALE; + ret = -1; + goto out; + } ret = lstat (real_path, &lstatbuf); @@ -819,6 +826,13 @@ posix_get_file_contents (xlator_t *this, uuid_t pargfid, MAKE_HANDLE_PATH (real_path, this, pargfid, name); + if (!real_path) { + op_ret = -ESTALE; + gf_log (this->name, GF_LOG_ERROR, + "Failed to create handle path for %s/%s", + uuid_utoa (pargfid), name); + goto out; + } op_ret = posix_istat (this, pargfid, name, &stbuf); if (op_ret == -1) { @@ -1018,10 +1032,12 @@ del_stale_dir_handle (xlator_t *this, uuid_t gfid) } size = posix_handle_path (this, gfid, NULL, newpath, sizeof (newpath)); - if (size <= 0 && errno == ENOENT) { - gf_log (this->name, GF_LOG_DEBUG, "%s: %s", newpath, - strerror (ENOENT)); - stale = _gf_true; + if (size <= 0) { + if (errno == ENOENT) { + gf_log (this->name, GF_LOG_DEBUG, "%s: %s", newpath, + strerror (ENOENT)); + stale = _gf_true; + } goto out; } @@ -1380,6 +1396,13 @@ __posix_fd_ctx_get (fd_t *fd, xlator_t *this, struct posix_fd **pfd_p) goto out; MAKE_HANDLE_PATH (real_path, this, fd->inode->gfid, NULL); + if (!real_path) { + gf_log (this->name, GF_LOG_ERROR, + "Failed to create handle path (%s)", + uuid_utoa (fd->inode->gfid)); + ret = -1; + goto out; + } pfd = GF_CALLOC (1, sizeof (*pfd), gf_posix_mt_posix_fd); if (!pfd) { diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c index 18ff857..b712ab3 100644 --- a/xlators/storage/posix/src/posix.c +++ b/xlators/storage/posix/src/posix.c @@ -147,7 +147,8 @@ posix_lookup (call_frame_t *frame, xlator_t *this, if (op_errno != ENOENT) { gf_log (this->name, GF_LOG_ERROR, "lstat on %s failed: %s", - real_path, strerror (op_errno)); + real_path ? real_path : "null", + strerror (op_errno)); } entry_ret = -1; @@ -243,7 +244,8 @@ posix_stat (call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *xdata) op_errno = errno; gf_log (this->name, (op_errno == ENOENT)? GF_LOG_DEBUG:GF_LOG_ERROR, - "lstat on %s failed: %s", real_path, + "lstat on %s failed: %s", + real_path ? real_path : "<null>", strerror (op_errno)); goto out; } @@ -378,8 +380,8 @@ posix_setattr (call_frame_t *frame, xlator_t *this, if (op_ret == -1) { op_errno = errno; gf_log (this->name, GF_LOG_ERROR, - "setattr (lstat) on %s failed: %s", real_path, - strerror (op_errno)); + "setattr (lstat) on %s failed: %s", + real_path ? real_path : "<null>", strerror (op_errno)); goto out; } @@ -905,6 +907,10 @@ posix_opendir (call_frame_t *frame, xlator_t *this, SET_FS_ID (frame->root->uid, frame->root->gid); MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + op_errno = ESTALE; + goto out; + } op_ret = -1; dir = opendir (real_path); @@ -1025,7 +1031,8 @@ posix_readlink (call_frame_t *frame, xlator_t *this, if (op_ret == -1) { op_errno = errno; gf_log (this->name, GF_LOG_ERROR, - "lstat on %s failed: %s", real_path, + "lstat on %s failed: %s", + loc->path ? loc->path : "<null>", strerror (op_errno)); goto out; } @@ -1084,6 +1091,13 @@ posix_mknod (call_frame_t *frame, xlator_t *this, SET_FS_ID (frame->root->uid, gid); + if (!real_path || !par_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } + + op_ret = posix_pstat (this, loc->pargfid, par_path, &preparent); if (op_ret == -1) { op_errno = errno; @@ -1276,6 +1290,11 @@ posix_mkdir (call_frame_t *frame, xlator_t *this, VALIDATE_OR_GOTO (priv, out); MAKE_ENTRY_HANDLE (real_path, par_path, this, loc, NULL); + if (!real_path || !par_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } gid = frame->root->gid; @@ -1466,6 +1485,11 @@ posix_unlink (call_frame_t *frame, xlator_t *this, SET_FS_ID (frame->root->uid, frame->root->gid); MAKE_ENTRY_HANDLE (real_path, par_path, this, loc, &stbuf); + if (!real_path || !par_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } op_ret = posix_pstat (this, loc->pargfid, par_path, &preparent); if (op_ret == -1) { @@ -1634,6 +1658,11 @@ posix_rmdir (call_frame_t *frame, xlator_t *this, priv = this->private; MAKE_ENTRY_HANDLE (real_path, par_path, this, loc, &stbuf); + if (!real_path || !par_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } op_ret = posix_pstat (this, loc->pargfid, par_path, &preparent); if (op_ret == -1) { @@ -1730,6 +1759,11 @@ posix_symlink (call_frame_t *frame, xlator_t *this, MAKE_ENTRY_HANDLE (real_path, par_path, this, loc, &stbuf); gid = frame->root->gid; + if (!real_path || !par_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } SET_FS_ID (frame->root->uid, gid); @@ -1874,7 +1908,18 @@ posix_rename (call_frame_t *frame, xlator_t *this, SET_FS_ID (frame->root->uid, frame->root->gid); MAKE_ENTRY_HANDLE (real_oldpath, par_oldpath, this, oldloc, NULL); + if (!real_oldpath || !par_oldpath) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } + MAKE_ENTRY_HANDLE (real_newpath, par_newpath, this, newloc, &stbuf); + if (!real_newpath || !par_newpath) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } op_ret = posix_pstat (this, oldloc->pargfid, par_oldpath, &preoldparent); if (op_ret == -1) { @@ -2067,8 +2112,17 @@ posix_link (call_frame_t *frame, xlator_t *this, SET_FS_ID (frame->root->uid, frame->root->gid); MAKE_INODE_HANDLE (real_oldpath, this, oldloc, &stbuf); + if (!real_oldpath) { + op_errno = errno; + goto out; + } MAKE_ENTRY_HANDLE (real_newpath, par_newpath, this, newloc, &stbuf); + if (!real_newpath || !par_newpath) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } op_ret = posix_pstat (this, newloc->pargfid, par_newpath, &preparent); if (op_ret == -1) { @@ -2174,7 +2228,7 @@ posix_truncate (call_frame_t *frame, xlator_t *this, loc_t *loc, off_t offset, op_errno = errno; gf_log (this->name, GF_LOG_ERROR, "pre-operation lstat on %s failed: %s", - real_path, strerror (op_errno)); + real_path ? real_path : "<null>", strerror (op_errno)); goto out; } @@ -2246,6 +2300,11 @@ posix_create (call_frame_t *frame, xlator_t *this, gid = frame->root->gid; SET_FS_ID (frame->root->uid, gid); + if (!real_path || !par_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } op_ret = posix_pstat (this, loc->pargfid, par_path, &preparent); if (op_ret == -1) { @@ -2421,6 +2480,11 @@ posix_open (call_frame_t *frame, xlator_t *this, VALIDATE_OR_GOTO (priv, out); MAKE_INODE_HANDLE (real_path, this, loc, &stbuf); + if (!real_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } if (IA_ISLNK (stbuf.ia_type)) { op_ret = -1; @@ -2842,6 +2906,11 @@ posix_statfs (call_frame_t *frame, xlator_t *this, VALIDATE_OR_GOTO (this->private, out); MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } priv = this->private; @@ -3116,6 +3185,11 @@ posix_setxattr (call_frame_t *frame, xlator_t *this, VALIDATE_OR_GOTO (dict, out); MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } op_ret = -1; dict_del (dict, GFID_XATTR_KEY); @@ -3157,6 +3231,9 @@ posix_xattr_get_real_filename (call_frame_t *frame, xlator_t *this, loc_t *loc, int op_ret = -1; MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + return -ESTALE; + } fd = opendir (real_path); if (!fd) @@ -3375,7 +3452,10 @@ posix_get_ancestry_non_directory (xlator_t *this, inode_t *leaf_inode, uuid_copy (loc->gfid, leaf_inode->gfid); MAKE_INODE_HANDLE (leaf_path, this, loc, NULL); - + if (!leaf_path) { + GF_FREE (loc); + goto out; + } GF_FREE (loc); size = sys_llistxattr (leaf_path, NULL, 0); @@ -4217,6 +4297,12 @@ posix_removexattr (call_frame_t *frame, xlator_t *this, DECLARE_OLD_FS_ID_VAR; MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + op_ret = -1; + op_errno = ESTALE; + goto out; + } + if (!strcmp (GFID_XATTR_KEY, name)) { gf_log (this->name, GF_LOG_WARNING, "Remove xattr called" @@ -4642,6 +4728,11 @@ posix_access (call_frame_t *frame, xlator_t *this, VALIDATE_OR_GOTO (loc, out); MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + op_ret = -1; + op_errno = errno; + goto out; + } op_ret = access (real_path, mask & 07); if (op_ret == -1) { @@ -4887,8 +4978,20 @@ posix_fill_readdir (fd_t *fd, DIR *dir, off_t off, size_t size, if (skip_dirs) { len = posix_handle_path (this, fd->inode->gfid, NULL, NULL, 0); + if (len <= 0) { + errno = ESTALE; + count = -1; + goto out; + } hpath = alloca (len + 256); /* NAME_MAX */ - posix_handle_path (this, fd->inode->gfid, NULL, hpath, len); + + if (posix_handle_path (this, fd->inode->gfid, NULL, hpath, + len) <= 0) { + errno = ESTALE; + count = -1; + goto out; + } + len = strlen (hpath); hpath[len] = '/'; } @@ -5033,7 +5136,13 @@ posix_entry_xattr_fill (xlator_t *this, inode_t *inode, tmp_loc.inode = inode; MAKE_HANDLE_PATH (entry_path, this, fd->inode->gfid, name); + if (!entry_path) { + gf_log (this->name, GF_LOG_WARNING, + "Failed to create handle path for %s (%s)", + name, uuid_utoa (fd->inode->gfid)); + return NULL; + } return posix_lookup_xattr_fill (this, entry_path, &tmp_loc, dict, stbuf); @@ -5057,8 +5166,11 @@ posix_readdirp_fill (xlator_t *this, fd_t *fd, gf_dirent_t *entries, dict_t *dic itable = fd->inode->table; len = posix_handle_path (this, fd->inode->gfid, NULL, NULL, 0); + if (len <= 0) + return -1; hpath = alloca (len + 256); /* NAME_MAX */ - posix_handle_path (this, fd->inode->gfid, NULL, hpath, len); + if (posix_handle_path (this, fd->inode->gfid, NULL, hpath, len) <= 0) + return -1; len = strlen (hpath); hpath[len] = '/'; @@ -5089,7 +5201,8 @@ posix_readdirp_fill (xlator_t *this, fd_t *fd, gf_dirent_t *entries, dict_t *dic posix_entry_xattr_fill (this, entry->inode, fd, entry->d_name, dict, &stbuf); - dict_ref (entry->dict); + if (entry->dict) + dict_ref (entry->dict); } entry->d_stat = stbuf; |