Date: Thu, 5 Jun 2014 02:20:24 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pietro Cerutti <gahr@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r267027 - head/usr.bin/users Message-ID: <20140605013835.M1934@besplex.bde.org> In-Reply-To: <201406032059.s53KxQAp081243@svn.freebsd.org> References: <201406032059.s53KxQAp081243@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 3 Jun 2014, Pietro Cerutti wrote:
> Log:
> - Avoid calling a wrapper function around strcmp
This changes correct code to give undefined behaviour.
> - Use sizeof(*array) instead of sizeof(element) everywhere
This also allows removal of a typedef obfuscation.
> Modified: head/usr.bin/users/users.c
> ==============================================================================
> --- head/usr.bin/users/users.c Tue Jun 3 20:58:11 2014 (r267026)
> +++ head/usr.bin/users/users.c Tue Jun 3 20:59:26 2014 (r267027)
> @@ -50,9 +50,9 @@ static const char rcsid[] =
> #include <unistd.h>
> #include <utmpx.h>
>
> -typedef char namebuf[sizeof(((struct utmpx *)0)->ut_user) + 1];
> +typedef char namebuf[sizeof(((struct utmpx *)0)->ut_user) + 1];
> +typedef int (*scmp)(const void *, const void *);
Most typedefs shouldn't exist, and these 2 are no exceptions.
'namebuf' only existed to reduce 2 copies of a type declaration to 1.
It obfuscated both. Now it is only used once.
'scmp' only exists to help implement undefined behaviour.
The change also de-KNFizes the formatting (from tab to space after the
first part of the type).
> @@ -91,7 +91,7 @@ main(int argc, char **argv)
> }
> endutxent();
> if (ncnt > 0) {
> - qsort(names, ncnt, sizeof(namebuf), scmp);
> + qsort(names, ncnt, sizeof(*names), (scmp)strcmp);
qsort()'s function pointer must point to a function like scmp, not
a different type of function with the type mismatch hidden by a bogus
cast.
The type puns fail as follow:
- strcmp is initially a function
- dereferencing it gives a pointer to a function of the type of strcmp
- casting this pointer to (scmp) gives a pointer to a different type
of function. The behaviour is defined. The representation may
change.
- use of such a cast function pointer to call the function is undefined
unless the pointer is converted back to the original (pointer) type
before making the call. Since qsort() cannot possibly know the original
type, it cannot convert back.
It is a feature that qsort()'s API doesn't have a function pointer typedef.
There are a few legitimate uses for function pointer typedefs, but in
practices most uses are for bogus casts to hide undefined behaviour, as
above.
The qsort() call also uses the other type obfuscation. 'namebuf' is
the typedef-name. This typedef name was only used twice, first to
declare 'names' and also here. Now with the better spelling of the
sizeof(), the typedef name is only used once. It just obfuscates
the declaration of 'names'.
> ...
> @@ -107,10 +107,3 @@ usage(void)
> fprintf(stderr, "usage: users\n");
> exit(1);
> }
> -
> -int
> -scmp(const void *p, const void *q)
> -{
> -
> - return (strcmp(p, q));
> -}
Most qsort() functions have to look like this, or a bit more complicated,
to avoid the undefined behaviour. This one looks simpler than it is.
It has to convert the arg types to those of strcmp(), but this happens
automatically due to strcmp()s prototype. The arg types started as
strings, but had to be converted to const void *'s to match qsort()'s
API, then have to be converted back to strings here.
The undefined behaviour is usually to work in practice because only
Vax hardware is supported. strings are so similar to void * that it
is hard to tell if the behaviour can ever be undefined in practice
for type puns between them. But the behaviour is more clearly undefined
for function pointers because compatibility of void * and char * as
args doesn't extend to the function pointers. The implementation could
reasonably check all types at runtime and is only constrained from
generating a fatal error for type mismatches for certain mismatches
that are specified to work for compatibility reasons. I think that
if the bogus conversions get as far as calling the function, then
the behaviour is defined by the compatibility kludges:
- string args became const void * in qsort()'s internals
- they are not converted back when they are passed to strcmp()
- however, the compatibility kludges now apply. const void * looks
enough like const char * to work. (I think it may have a different
representation, but only with bits that aren't really used, for
example extra type bits that might be used for error checking
but must not be used here due to the compatibility kludges.)
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140605013835.M1934>