Date: Mon, 18 Mar 2013 16:51:22 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, freebsd-arch@FreeBSD.org Subject: Re: chflags(2)'s flags argument. Message-ID: <20130318155059.V925@besplex.bde.org> In-Reply-To: <20130317211027.GJ1364@garage.freebsd.pl> References: <20130317003559.GA1364@garage.freebsd.pl> <20130317064123.GM3794@kib.kiev.ua> <20130317111112.GC1364@garage.freebsd.pl> <20130317155743.GR3794@kib.kiev.ua> <20130317162021.GG1364@garage.freebsd.pl> <20130317162533.GT3794@kib.kiev.ua> <20130317164632.GI1364@garage.freebsd.pl> <20130317174205.GU3794@kib.kiev.ua> <20130317211027.GJ1364@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 17 Mar 2013, Pawel Jakub Dawidek wrote:
> On Sun, Mar 17, 2013 at 07:42:05PM +0200, Konstantin Belousov wrote:
>> I would say that API and ABI stability is more important then the
>> consistency. I think we are in the agreement of changing the kernel
>> interface for the chflags/fchflags to use long for flags.
>>
>> For lchflags, my opinion is that the best would be not to change it.
>
> ABI is out of question, we do preserve its stability. And as for API,
> I think it is really low price to pay (if any) for consistency and would
> also allow us to get rid of hacks like the one in chflags(1):
I think it is possible to just change it. The correct API is of course
that the function arg type is the default promotion of the type of
st_flags. Fortunately this is the same (u_int) on all supported arches.
>
> int (*change_flags)(const char *, unsigned long);
> [...]
> /* XXX: Why don't chflags and lchflags have compatible prototypes? */
> if (hflag)
> change_flags = (int (*)(const char *, unsigned long))lchflags;
> else
> change_flags = chflags;
Why do people write garbage like that? This hack is not necessary. It
just uses bogus casts to break the warning about undefined behaviour when
the pointer is used. The cast is valid, but the function pointer
created by it is only usable if it is converted back to the type of
the original function. That is not done, so the result is undefined.
(This result is whatever happens when the lchflags() function is passed
a u_long arg instead of the int arg that it expects. The kernel uses
similar type puns with more reason, and works less accidentally.)
Fixing this to use the pointer correctly takes slightly more code than
just using the unpointed-to functions directly, not counting the 6 times
as much code needed for the garbage:
@ diff -u2 chflags.c~ chflags.c
@ --- chflags.c~ 2010-12-26 17:19:39.000000000 +0000
@ +++ chflags.c 2013-03-18 04:43:45.515645000 +0000
@ @@ -66,5 +66,4 @@
@ int ch, fts_options, oct, rval;
@ char *flags, *ep;
@ - int (*change_flags)(const char *, unsigned long);
@
@ Hflag = Lflag = Rflag = fflag = hflag = vflag = 0;
@ @@ -118,10 +117,4 @@
@ fts_options = hflag ? FTS_PHYSICAL : FTS_LOGICAL;
@
@ - /* XXX: Why don't chflags and lchflags have compatible prototypes? */
@ - if (hflag)
@ - change_flags = (int (*)(const char *, unsigned long))lchflags;
@ - else
@ - change_flags = chflags;
@ -
@ flags = *argv;
@ if (*flags >= '0' && *flags <= '7') {
@ @@ -180,5 +173,6 @@
@ if (newflags == p->fts_statp->st_flags)
@ continue;
@ - if ((*change_flags)(p->fts_accpath, newflags) && !fflag) {
@ + if ((hflag ? lchflags(p->fts_accpath, newflags) :
@ + chflags(p->fts_accpath, newflags)) & !fflag) {
@ warn("%s", p->fts_path);
@ rval = 1;
cp/utils.c already uses correct code. It uses a 3-way selection between
fchflags, lchflags and chflags, and fchflags is too obviously different
to be corruptible using a pointer as above. (The 3-way selection is
bogus, however. The chflags case in it is unreachable, at least with
other changes, since the case of a non-link is or should always be
handled using a file descriptor. Using a file descriptor reduces races
but is unfortunately impossible for symlinks.)
> In common case when program just calls lchflags() directly it won't even
> notice API change.
And in the uncommon case, broken code like the above won't even notice
that the API change unbreaks it, since the brokenness must have been
being for the broken code to appear to work. Except the unbreakage
would prevent the compiler reporting the above as garbage. (The
lifetime analysis for the pointer is simple, so it is fairly easy to
see that it is always garbage when it is used with hflag != 0. The
compiler could, instead of reporting this, validly optimize away that
case.)
> Maybe we could arrange ports build with lchflags(2) changed to take
> unsigned long to see how destructive the change really is, because my
> expectation is that not very.
Why not use the correct type (u_int)? u_long is just too long, since
st_flags is only 32 bits, so much larger and realler API and ABI changes
would be needed to support more than 32 flags.
The original poorly chosen u_long types are probably due to 4.4BSD
having vestiges of support for 16-bit ints. Old BSDs used long for
almost everthing that might be large. For example, pid_t was long
in 4.4BSD-Lite1. This mainly caused ABI problems on 64-bit arches,
so NetBSD changed most of these longs to int32_t. 4.4BSD-Lite1 picked
up many of these changes, and FreeBSD got them from Lite2. But
NetBSD didn't change the u_longs in [f]chflags(). FreeBSD enlarged
the mess by importing lchflags() from NetBSD and changing its arg
type to int.
The garbage also has some style bugs. It spells "unsigned long" as
itself. All other code in chflags.c still uses the normal abbreviation
u_long.
Now I remember where the hack came from. It is not from cp/utils.c,
but from copying similar pointer code in chmod/chmod.c and chown/chown.c
There using the pointer is just verbose minor obfuscation. My version
cleans up chmod as follows:
@ diff -u2 chmod.c~ chmod.c
@ --- chmod.c~ Wed Apr 7 20:21:29 2004
@ +++ chmod.c Wed Apr 7 20:21:30 2004
@ @@ -66,5 +65,4 @@
@ char *mode;
@ mode_t newmode;
@ - int (*change_mode)(const char *, mode_t);
@
@ set = NULL;
@ @@ -140,9 +145,4 @@
@ fts_options = hflag ? FTS_PHYSICAL : FTS_LOGICAL;
@
@ - if (hflag)
@ - change_mode = lchmod;
@ - else
@ - change_mode = chmod;
@ -
@ mode = *argv;
@ if ((set = setmode(mode)) == NULL)
@ @@ -183,11 +182,12 @@
@ if ((newmode & ALLPERMS) == (p->fts_statp->st_mode & ALLPERMS))
@ continue;
@ - if ((*change_mode)(p->fts_accpath, newmode) && !fflag) {
@ - warn("%s", p->fts_path);
@ - rval = 1;
@ + if ((hflag ? lchmod : chmod)(p->fts_accpath, newmode)) {
@ + if (!fflag) {
@ + warn("%s", p->fts_path);
@ + rval = 1;
@ + }
@ } else {
@ if (vflag) {
@ (void)printf("%s", p->fts_path);
@ -
@ if (vflag > 1) {
@ char m1[12], m2[12];
-current does this differently. It has the same fflag fix. It doesn't
have the vflag cleanup. It uses different verboseness for the hflag part:
if (hflag)
error = lchmod(p->fts_accpath, newmode);
else
error = chmod(p->fts_accpath, newmode);
if (error) ...
lchflags()'s different prototype prevents using the conditional operator
cleanly, but using it is still better (2 lines instead of 5, with clearer
logic).
chown/chown.c in -current does all this cleanly. Only its nearby printing
for vflag is ugly.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130318155059.V925>