Date: Thu, 5 Jun 2014 12:11:48 +0200 From: Pietro Cerutti <gahr@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: bapt@FreeBSD.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Julian Elischer <julian@freebsd.org>, cognet@FreeBSD.org Subject: Re: svn commit: r267027 - head/usr.bin/users Message-ID: <20140605101148.GJ37870@ptrcrt.ch> In-Reply-To: <20140605013835.M1934@besplex.bde.org> References: <201406032059.s53KxQAp081243@svn.freebsd.org> <20140605013835.M1934@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--zOcTNEe3AzgCmdo9
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Bruce,
your comments do make sense. I semi-seriously suggest that we get rid of
the current implementation and replace it with this. Comments?
/*
* Copyright (c) 2014 Pietro Cerutti <gahr@FreeBSD.org>
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
* 4. Neither the name of the University nor the names of its contributors
* may be used to endorse or promote products derived from this software
* without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPO=
SE
* ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTI=
AL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRI=
CT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD: head/usr.bin/users/users.c 267027 2014-06-03 20:59:26Z =
gahr $");
#include <utmpx.h>
#include <algorithm>
#include <cstdlib>
#include <iostream>
#include <iterator>
#include <string>
#include <vector>
using namespace std;
int
main(int argc, char **)
{
struct utmpx *ut;
vector<string> names;
if (argc > 1) {
cerr << "usage: users" << endl;
return (1);
}
setutxent();
while ((ut =3D getutxent()) !=3D NULL) {
if (ut->ut_type !=3D USER_PROCESS)
continue;
names.push_back(ut->ut_user);
}
endutxent();
if (names.size() =3D=3D 0) {
return (0);
}
sort(begin(names), end(names));
vector<string>::iterator last(unique(begin(names), end(names)));
copy(begin(names), last-1, ostream_iterator<string>(cout, " "));
cout << *(last-1) << endl;
}
On 2014-Jun-05, 02:20, Bruce Evans wrote:
> On Tue, 3 Jun 2014, Pietro Cerutti wrote:
>=20
> > Log:
> > - Avoid calling a wrapper function around strcmp
>=20
> This changes correct code to give undefined behaviour.
>=20
> > - Use sizeof(*array) instead of sizeof(element) everywhere
>=20
> This also allows removal of a typedef obfuscation.
>=20
> > Modified: head/usr.bin/users/users.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> > --- 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[] =3D
> > #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 *);
>=20
> Most typedefs shouldn't exist, and these 2 are no exceptions.
>=20
> 'namebuf' only existed to reduce 2 copies of a type declaration to 1.
> It obfuscated both. Now it is only used once.
>=20
> 'scmp' only exists to help implement undefined behaviour.
>=20
> The change also de-KNFizes the formatting (from tab to space after the
> first part of the type).
>=20
> > @@ -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);
>=20
> 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.
>=20
> 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 origin=
al
> type, it cannot convert back.
>=20
> It is a feature that qsort()'s API doesn't have a function pointer typede=
f.
> 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.
>=20
> 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'.
>=20
> > ...
> > @@ -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));
> > -}
>=20
> 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.
>=20
> 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.)
>=20
> Bruce
--=20
Pietro Cerutti
The FreeBSD Project
gahr@FreeBSD.org
PGP Public Key:
http://gahr.ch/pgp
--zOcTNEe3AzgCmdo9
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQJ8BAEBCgBmBQJTkEJkXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXREQTZERTEwNkE1Qjg1NEI4NUREODZENDlB
REQwRDM4RUExOTIwODlFAAoJEK3Q046hkgieQF4P/AmNjl1Bp7L1dex3jcmhntm6
VefBMgUNyhIvmXs/yu5BqYeshlHrHNpcQtwqQ8oLWVlmRn6DQpiOLvvdpvgvOe8A
8cGkTH9l9tPqwghV9fn/tN0p04w8lbvhNZCshaSZ0oE8X+jECwc94ilfxkfQDv9Q
8J7sRiyfp2D8kAM8/zsKHM//QoD5bbvi1eHiAPnKt3SSAwCQ6EGA8qU3Ou42WAFQ
QjRyOZ9CBkRRwik/f60rvBDqRq5Gpxokzd9m4Hsl25CjH/RdlzQiaON+out7KWEK
zuHvFrWUr5tFs566cQV/ETCyHucHBq6APDS+BOd3VChka9uUQUk+PuXzqrYQHWP/
i1dgthO8b8P0eX+XnCgFog3t9BPD25khM5Nfd7YYKwM/dcbg+cN09JPU/Onmi2FP
H0mEvHWdshUTPl4y9QL5XbsecIIjLZhdEM4zW1Ij3+bb3NecTKGNtIJY1Q5Hk6tK
1ewgi1jRybq+oX67eGGpQdfy+LJKHYmoF8+XLJ39Ltn977NPIcJzFyvy0jV7arlZ
DxUuRagUCl72G0iXpSuaTU3iddjCBRn0ZgA7n0Q3GJjbBRj0Jm78uV9JeljJ7/Vd
ukDREHXung18HxmHJTMXBZEDriX20BDVs5pBCzon3YhvAFa+FFBbMZ13nTPBwgEH
hFQeUVeRiJMYvy90Wc2y
=LX0s
-----END PGP SIGNATURE-----
--zOcTNEe3AzgCmdo9--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140605101148.GJ37870>