Date: Mon, 4 Jul 2011 20:48:28 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Stefan Esser <se@FreeBSD.org> Cc: Garrett Wollman <wollman@csail.mit.edu>, freebsd-standards@FreeBSD.org Subject: Re: New patches for expr (was: Re: Fwd: [RFC] Consistent numeric range for "expr" on all architectures) Message-ID: <20110704195515.A1938@besplex.bde.org> In-Reply-To: <4E0E2535.1060204@freebsd.org> References: <4E09AF8E.5010509@freebsd.org> <4E0B860E.50504@freebsd.org> <20110630164653.GA82980@zim.MIT.EDU> <4E0CACFB.8040906@freebsd.org> <19980.47094.184089.398324@khavrinen.csail.mit.edu> <20110701092909.F1164@besplex.bde.org> <4E0E2535.1060204@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 1 Jul 2011, Stefan Esser wrote:
> On 01.07.2011 01:58, Bruce Evans wrote:
> [...]
>> The part about recognizing different syntaxes is an extension and
>> requires explicit permission. The part about "using" a rank larger
>> than that of signed long requires no explicit permission, since it
>> only affects the result if there is overflow, but then the behaviour
>> is undefined. So any utility can do the latter.
>
> I have attached a new patch which does the following for overflow:
>
> 1) Numeric range is extended to intmax_t
> 2) Overflow is caught in all arithmetic operations
> 3) Integers not used in arithmetic operations are treated like strings
>> ...
> 4) Leading whitespace in numbers is only accepted in compat mode
> ("expr -e" or EXPR_COMPAT defined in the process environment or
> check_utility_compat("expr") == true).
>
> <<<<<< Aside: Is check_utility_compat() still used at all?
>
> It provides compatibility with FreeBSD-4 if the environment variable
> _COMPAT_FreeBSD_4 exists and contains "expr" as member of a comma
> separated list, or if the following symlink exists in /etc:
>
> compat-FreeBSD-4-util -> expr
>
> In fact, the symlink is also parsed as comma separated list, but expr is
> the only program in FreeBSD that calls check_utility_compat() ...
>
> Since expr is the only user of check_utility_compat() and the same
> effect can be had by specifying EXPR_COMPAT, the check_utility_compat
> function should be considered deprecated and only be kept in libc for
> backwards compatibility of old binaries, IMHO. This function adds one
> unnecessary call of getenv() and one of readlink() per invocation of
> expr and adds complexity to the man-page for expr.
I'd like to remove the symlink. The symlink for malloc() annoys me
too. It pessimizes and bloats malloc() a little, and on FreeBSD cluster
machines, it breaks support for old binaries by being misconfigured
to contain flags that are incompatible with ones recognized by old
versions of malloc(). And there is no way to prevent even current
malloc() from examining the symlink. It examines the symlink first
and spams stderr with messages about any invalid flags in the symlink.
Then it examines the environment variable, and the environment variable
can be used to cancel any unportable or otherwise unwanted flags in
the symlink, but the messages cannot be canceled, and more bogus
messages may be printed to defeat the next stage. Then it examines
the compile-time variable, and this can't completely cancel the previous
stages, due to the messages.
> I have prepared a number of patches, which I attach to this message. All
> patches are relative to the current SVN version of expr.y (i.e. do *not*
> try to apply the higher numbered ones on a patched source file).
>
> The first file (expr.y.diff1) contains a "diff -w -u3" of the functional
> changes that I suggest. It is only meant to show the actual code changes
> (applying this diff results in mis-indented code).
>
> The second file (expr.y.diff2) has been created with the same command
> and contains some refactoring of functions. It is functionally identical
> to the first version.
>
> The third file (expr.y.diff3) contains the diff with whitespace and
> style changes that I suggest as final version. I plan to commit the
> changes in sequence (with correct indentation of each step, of course),
> in order to keep the change history separate for each intended purpose.
The patches seem OK, except:
- I don't like reversing the semantics of the -e option from "extension"
to "compatible"
- '+<integer>' is now not accepted even in compatibility mode. Accepting
'+<integer>' wasn't documented, but was a side effect of using strto*()
for parsing
- the changes to the man page aren't in the patches.
> I also include a SHAR of regression tests meant to be committed to
> /usr/src/tools/regression/bin/expr. All tests pass with the current
> patched version of expr (while the currently shipped version dumps core
> in one test case).
I ran the regression tests on a 2005 version of NetBSD expr and a 2002
version of Linux expr. There were the following failures:
NetBSD:
% not ok 10 - - (got 0, expected 2)
% not ok 11 -1 - (got 0, expected 2)
% not ok 12 -1 + 1 - (got 1, expected 2)
% not ok 14 " 1" + 1 - (got 0, expected 2)
% not ok 15 -e " 1" + 1 - (got 2, expected 0)
% not ok 29 -- -9223372036854775808 / -1 - (got 0, expected 2)
NetBSD does sloppy parsing using strtoll(), and does all numeric operations
in type int64_t, and does primitive range checking. Most of the failures
are from the sloppy parsing. #29 is from the range checking being too
primitive for the delicate case INTMAX_MIN / -1.
Linux:
% not ok 10 - - (got 0, expected 2)
% not ok 11 -1 - (got 0, expected 2)
% not ok 12 -1 + 1 - (got 1, expected 2)
I don't understand these 3. Linux expr gives the error when run directly.
Now I'm confused by the results of NetBSD expr for these tests too.
% not ok 15 -e " 1" + 1 - (got 2, expected 0)
No -e in Linux.
% not ok 16 "" + 1 - (got 2, expected 0)
The test seems to be wrong here. The empty string should only be accepted
in compat mode, but this test seems to use the default mode.
% not ok 18 9223372036854775807 + 1 - (got 0, expected 2)
% not ok 21 -- -9223372036854775808 \* -1 - (got 0, expected 2)
% not ok 24 10000000000000000000 + 0 - (got 0, expected 2)
% not ok 29 -- -9223372036854775808 / -1 - (got 0, expected 2)
% not ok 32 4611686018427387904 \* 2 - (got 0, expected 2)
% not ok 35 4611686018427387905 \* -2 - (got 0, expected 2)
Since Linux does no range checking.
% not ok 55 " 1" + 1 - (got 2, expected 0)
Since Linux doesn't support EXPR_COMPAT=1 (to allow leading spaces and
no digits). This shows an evil of environment variables -- it is not
obvious what is tested since the environment variable has a critical
effect but is not mentioned in the test output.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110704195515.A1938>