Date: Wed, 6 Oct 2010 16:03:00 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: Sergey Kandaurov <pluknet@gmail.com> Cc: Alexander Best <arundel@freebsd.org>, freebsd-hackers@freebsd.org Subject: Re: issue with unsetting 'arch' flag Message-ID: <AANLkTimut3obh4VgKVv3PCgicwEKK4f0zg=W2OnSv86s@mail.gmail.com> In-Reply-To: <AANLkTikYX0vsxZi=J6Asekk-Kd_Y4MyemjDxM5FXARng@mail.gmail.com> References: <20101005235054.GA45827@freebsd.org> <AANLkTi=sA4GP=B61tbEmG6B0CYcET=dCFMJByoS_5=yi@mail.gmail.com> <20101006173522.GA92402@freebsd.org> <AANLkTi==F4zFmJxqOBzMCk%2Buci6XbvoQBe4mqxHjtbr6@mail.gmail.com> <20101006193827.GA13528@freebsd.org> <AANLkTikYX0vsxZi=J6Asekk-Kd_Y4MyemjDxM5FXARng@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--0050450157037235620491fac75b
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
On Wed, Oct 6, 2010 at 3:01 PM, Sergey Kandaurov <pluknet@gmail.com> wrote:
> On 6 October 2010 23:38, Alexander Best <arundel@freebsd.org> wrote:
>> On Wed Oct =A06 10, Garrett Cooper wrote:
>>> On Wed, Oct 6, 2010 at 10:35 AM, Alexander Best <arundel@freebsd.org> w=
rote:
>>> > On Wed Oct =A06 10, Garrett Cooper wrote:
>>> >> On Tue, Oct 5, 2010 at 4:50 PM, Alexander Best <arundel@freebsd.org>=
wrote:
>>> >> > hi there,
>>> >> >
>>> >> > i think the following example shows the problem better than a long=
explanation:
>>> >> >
>>> >> > `touch ftest && chflags arch ftest && chflags -vv 0 ftest`.
>>> >> > =A0^^non-root =A0 =A0 ^^root =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^^non-=
root
>>> >> >
>>> >> > chflags claims to have cleared the 'arch' flag (which should be im=
possible as
>>> >> > non-root user), but indeed has done nothing.
>>> >> >
>>> >> > i've tried the same with 'sappnd' and that works as can be expecte=
d.
>>> >> >
>>> >> > The issue was confirmed to exist in HEAD (me), stable/8 (pgollucc1=
, jpaetzel)
>>> >> > and stable/7 (nox).
>>> >> > On stable/6 it does NOT exist (jpaetzel). chflags properly fails w=
ith EPERM.
>>> >>
>>> >> =A0 =A0 Fails for me when I call the syscall directly, as I would ex=
pect,
>>> >> and passes when I'm superuser:
>>> >>
>>> >> $ ./test_chflags
>>> >> (uid, euid) =3D (1000, 1000)
>>> >> test_chflags: chflags: Operation not permitted
>>> >> test_chflags: lchflags: Operation not permitted
>>> >> $ sudo ./test_chflags
>>> >> (uid, euid) =3D (0, 0)
>>> >>
>>> >> =A0 =A0 According to my basic inspection in strtofflags
>>> >> (.../lib/libc/gen/strtofflags.c), it works as well.
>>> >> =A0 =A0 And last but not least, executing the commands directly on t=
he CLI work:
>>> >>
>>> >> $ tmpfile=3D`mktemp /tmp/chflags.XXXXXX`
>>> >> $ chflags arch $tmpfile
>>> >> chflags: /tmp/chflags.nQm1IL: Operation not permitted
>>> >> $ rm $tmpfile
>>> >> $ tmpfile=3D`mktemp /tmp/chflags.XXXXXX`
>>> >> $ sudo chflags arch $tmpfile
>>> >> $ sudo chflags noarch $tmpfile
>>> >> $ rm $tmpfile
>>> >
>>> > thanks for your test app and helping out with this problem. i'm not s=
ure
>>> > however you understood the problem. probably i didn't explain it righ=
t:
>>> >
>>> > $ sudo rm -d /tmp/chflags.XXXXXX
>>> > $ tmpfile=3D`mktemp /tmp/chflags.XXXXXX`
>>> > $ sudo chflags arch $tmpfile
>>> > $ chflags noarch $tmpfile
>>> >
>>> > is what's causing the problem. the last chflags call should fail, but=
it
>>> > doesn't.
>>>
>>> Sorry... my CLI based example was stupid. I meant:
>>>
>>> $ tmpfile=3D`mktemp /tmp/chflags.XXXXXX`
>>> $ chflags arch $tmpfile
>>> chflags: /tmp/chflags.V2NpXR: Operation not permitted
>>> $ chflags noarch $tmpfile
>>> $ rm $tmpfile
>>>
>>> Currently chflags(2) states:
>>>
>>> =A0 =A0 =A0The SF_IMMUTABLE, SF_APPEND, SF_NOUNLINK, and SF_ARCHIVED fl=
ags may only
>>> =A0 =A0 =A0be set or unset by the super-user. =A0Attempts to set these =
flags by non-
>>> =A0 =A0 =A0super-users are rejected, >>> attempts by non-superusers to =
clear
>>> flags that
>>> =A0 =A0 =A0are already unset are silently ignored. <<< =A0These flags m=
ay be set at any
>>> =A0 =A0 =A0time, but normally may only be unset when the system is in s=
ingle-user
>>> =A0 =A0 =A0mode. =A0(See init(8) for details.)
>>>
>>> So this behavior is already well documented :). The EPERM section
>>> should really note SF_ARCHIVED though (whoever added the flag forgot
>>> to add that particular item to the ERRORS section).
>>
>> that's perfectly alright. clearing an unset flag shouldn't cause any err=
or to
>> be returned. however in my example arch *does* get set and still trying =
to
>> unset it as normal user doesn't return an error.
>>
>
> It's even more interesting.
>
> As far as I could parse the code:
> - UFS has no special handling for SF_ARCHIVED (I found only it for msdosf=
s)
_very_ interesting:
[/sys]$ grep -r SF_ARCHIVED kern/ fs/ ufs/ | grep -v svn
fs/msdosfs/msdosfs_vnops.c: vap->va_flags |=3D SF_ARCHIVED;
fs/msdosfs/msdosfs_vnops.c: if (vap->va_flags & ~SF_ARCHIVED)
fs/msdosfs/msdosfs_vnops.c: if (vap->va_flags & SF_ARCHIVED)
The commit that introduced this change probably wasn't doing the
right thing: http://svn.freebsd.org/viewvc/base/head/sys/fs/msdosfs/msdosfs=
_vnops.c?revision=3D5241&view=3Dmarkup
; cp(1) probably should have been fixed in lieu of `fixing' msdosfs.
> - ufs_setattr() does not handle unsetting SF_ARCHIVED,
> =A0so all what it does is simply return zero.
[EOPNOTSUPP] The underlying file system does not support file
flags.
So I would expect for invalid flags to return EOPNOTSUPP.
...
$ ~/test_chflags_negative
test_chflags_negative: should not get here
$ sudo ~/test_chflags_negative
test_chflags_negative: should not get here
*facepalm*
I think the problem in part is here (sys/stat.h):
*
* Super-user and owner changeable flags.
*/
#define UF_SETTABLE 0x0000ffff /* mask of owner changeable flags *=
/
#define UF_NODUMP 0x00000001 /* do not dump file */
#define UF_IMMUTABLE 0x00000002 /* file may not be changed */
#define UF_APPEND 0x00000004 /* writes to file may only append *=
/
#define UF_OPAQUE 0x00000008 /* directory is opaque wrt. union *=
/
#define UF_NOUNLINK 0x00000010 /* file may not be removed or renam=
ed */
/*
* Super-user changeable flags.
*/
#define SF_SETTABLE 0xffff0000 /* mask of superuser changeable fla=
gs */
#define SF_ARCHIVED 0x00010000 /* file is archived */
#define SF_IMMUTABLE 0x00020000 /* file may not be changed */
#define SF_APPEND 0x00040000 /* writes to file may only append *=
/
#define SF_NOUNLINK 0x00100000 /* file may not be removed or renam=
ed */
#define SF_SNAPSHOT 0x00200000 /* snapshot inode */
Note the *_SETTABLE macros, and the fact that they allow for more
functionality than what's currently slotted with the one-hot encoded
flags currently available.
SF_ARCHIVED is not present in the other BSDs or Mac OSX either (I
did some hunting for a python bug related to chflags a few weeks
ago)... and I'm not even sure what this functionality really buys us
because it's not well described (but I'd be happy to get an
explanation/history lesson).
> - /bin/chflags doesn't check the actual flags value from inode after
> calling chflags() syscall, and blindly assumes all is well, if chflags()
> returns with zero,
Yeah... but ideally tests should be written for this stuff and
exercised on all filesystems and exercised whenever code in this
particular path is changed, because that would potentially turn into a
noticeable performance hit [depending on how it's implemented in
chflags(1)]. And lo and behold it already does exist under
.../tools/regression/fstest/tests/chflags . I'll audit this once I get
back home...
Thanks,
-Garrett
--0050450157037235620491fac75b
Content-Type: application/octet-stream; name="test_chflags_negative.c"
Content-Disposition: attachment; filename="test_chflags_negative.c"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_geyss8wh0
I2luY2x1ZGUgPHN5cy9zdGF0Lmg+CiNpbmNsdWRlIDxlcnIuaD4KI2luY2x1ZGUgPHVuaXN0ZC5o
PgoKaW50Cm1haW4odm9pZCkKewoJaWYgKGNoZmxhZ3MoIi90bXAiLCAtMSkpIHsKCQl3YXJuKCJP
ayIpOwoJCXJldHVybiAoMCk7Cgl9Cgl3YXJueCgic2hvdWxkIG5vdCBnZXQgaGVyZSIpOwoJcmV0
dXJuICgxKTsKfQo=
--0050450157037235620491fac75b--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimut3obh4VgKVv3PCgicwEKK4f0zg=W2OnSv86s>