From 5b2d308f4b3969e37ceca7648165add1badb0de3 Mon Sep 17 00:00:00 2001 From: Ravishankar N Date: Wed, 29 Jan 2014 12:09:42 +0000 Subject: storage/posix: perform chmod after chown. Problem: When a replica brick is added to a volume, set-user-ID and set-group-ID permission bits of files are not set correctly in the new brick. The issue is in the posix_setattr() call where we do a chmod followed by a chown. But according to the man pages for chown: When the owner or group of an executable file are changed by an unprivileged user the S_ISUID and S_ISGID mode bits are cleared. POSIX does not specify whether this also should happen when root does the chown(). Fix: Swap the chmod and chown calls in posix_setattr() BUG: 1058797 Change-Id: Id2fbd8394cf6faf669f414775409f20f46009f2b Signed-off-by: Ravishankar N Reviewed-on: http://review.gluster.org/6988 Tested-by: Gluster Build System Reviewed-by: Vijay Bellur --- tests/bugs/bug-1058797.t | 45 +++++++++++++++++++++++++++++++++++++++ xlators/storage/posix/src/posix.c | 12 +++++------ 2 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 tests/bugs/bug-1058797.t diff --git a/tests/bugs/bug-1058797.t b/tests/bugs/bug-1058797.t new file mode 100644 index 000000000..2b80794cf --- /dev/null +++ b/tests/bugs/bug-1058797.t @@ -0,0 +1,45 @@ +#!/bin/bash +#Test that the setuid bit is healed correctly. + +. $(dirname $0)/../include.rc +. $(dirname $0)/../volume.rc + +cleanup; +#Basic checks +TEST glusterd + +#Create a 1x2 replica volume +TEST $CLI volume create $V0 replica 2 $H0:$B0/brick{0,1}; +TEST $CLI volume start $V0 +TEST $CLI volume set $V0 cluster.self-heal-daemon off + +# FUSE mount;create a file +TEST glusterfs -s $H0 --volfile-id $V0 $M0 +TEST touch $M0/file + +#Kill brick1 and set S_ISUID and S_ISGID bits from mount point +kill_brick $V0 $H0 $B0/brick1 +TEST chmod +x,+s $M0/file + +#Get file permissions from backend brick0 and verify that S_ISUID is indeed set +file_permissions1=`ls -l $B0/brick0/file | awk '{print $1}'| cut -d. -f1 | cut -d- -f2,3,4,5,6` +setuid_bit1=`echo $file_permissions1 | cut -b3` +EXPECT "s" echo $setuid_bit1 + +#Restart volume and do lookup from mount to trigger heal +TEST $CLI volume start $V0 force +EXPECT_WITHIN 20 "1" afr_child_up_status $V0 1 +TEST ls -l $M0/file + +#Get file permissions from healed brick1 and verify that S_ISUID is indeed set +file_permissions2=`ls -l $B0/brick1/file | awk '{print $1}' | cut -d. -f1 | cut -d- -f2,3,4,5,6` +setuid_bit2=`echo $file_permissions2 | cut -b3` +EXPECT "s" echo $setuid_bit2 + +#Also compare the entire permission string,just to be sure +EXPECT $file_permissions1 echo $file_permissions2 +TEST umount $M0 +TEST $CLI volume stop $V0 +TEST $CLI volume delete $V0; + +cleanup; diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c index 2abe92835..d134eceb8 100644 --- a/xlators/storage/posix/src/posix.c +++ b/xlators/storage/posix/src/posix.c @@ -362,23 +362,23 @@ posix_setattr (call_frame_t *frame, xlator_t *this, goto out; } - if (valid & GF_SET_ATTR_MODE) { - op_ret = posix_do_chmod (this, real_path, stbuf); + if (valid & (GF_SET_ATTR_UID | GF_SET_ATTR_GID)){ + op_ret = posix_do_chown (this, real_path, stbuf, valid); if (op_ret == -1) { op_errno = errno; gf_log (this->name, GF_LOG_ERROR, - "setattr (chmod) on %s failed: %s", real_path, + "setattr (chown) on %s failed: %s", real_path, strerror (op_errno)); goto out; } } - if (valid & (GF_SET_ATTR_UID | GF_SET_ATTR_GID)){ - op_ret = posix_do_chown (this, real_path, stbuf, valid); + if (valid & GF_SET_ATTR_MODE) { + op_ret = posix_do_chmod (this, real_path, stbuf); if (op_ret == -1) { op_errno = errno; gf_log (this->name, GF_LOG_ERROR, - "setattr (chown) on %s failed: %s", real_path, + "setattr (chmod) on %s failed: %s", real_path, strerror (op_errno)); goto out; } -- cgit