From owner-svn-src-all@FreeBSD.ORG Wed Jun 4 16:20:35 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DF9471A8; Wed, 4 Jun 2014 16:20:35 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 7040C23C5; Wed, 4 Jun 2014 16:20:35 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 03B0B1044667; Thu, 5 Jun 2014 02:20:32 +1000 (EST) Date: Thu, 5 Jun 2014 02:20:24 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pietro Cerutti Subject: Re: svn commit: r267027 - head/usr.bin/users In-Reply-To: <201406032059.s53KxQAp081243@svn.freebsd.org> Message-ID: <20140605013835.M1934@besplex.bde.org> References: <201406032059.s53KxQAp081243@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=U6SrU4bu c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=M9zwebxRaw4A:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=iRfTH0PG8UVemprl3JAA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Jun 2014 16:20:36 -0000 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 > #include > > -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