From c77999b600ea9c20cc23cf4bfc829c07438dcf6a Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sun, 21 Jan 2024 01:45:31 +0000 Subject: [PATCH] arith: avoid assignment triggering get/getn discipline An assignment should trigger the 'set' discipline, not the 'get'. Yet, when the assignment is in an arithmetic expression: $ x.set() { echo SET; }; x.get() { echo GET; }; ((x=1)) SET GET That is clearly incorrect, inconsistent and counter to the documentation. For a simple shell assignment 'x=1' instead of '((x=1))', only SET is triggered. The inconsistency also applies to internal shell disciplines. This can be demonstrated using the RANDOM reproducible pseudorandom sequence: $ ksh -c 'RANDOM=123; echo $RANDOM $RANDOM $RANDOM $RANDOM' 29031 19003 403 6924 $ ksh -c '((RANDOM=123)); echo $RANDOM $RANDOM $RANDOM $RANDOM' 19003 403 6924 25974 Note the second output is the same as the first one starting from the second number, meaning RANDOM was incorrectly read from while the arithmetic command was assigning the seed value to it. The second output should be identical to the first. Another reproducer of the same problem: $ (( r = (RANDOM = -1) )); echo $r 2100 The return value r of the "RANDOM = -1" assignment should be -1. No reading from RANDOM should happen here. Analysis: Since shell arithmetic uses C syntax, an assignment also has a return value, i.e., the value assigned to the variable. In the arith() function in arith.c, case ASSIGN, there is a 'r=nv_getnum(np)' call that retrieves this value. However, calling nv_getnum() also triggers a getn/get discipline, which is the bug. Simply replacing this call with "r = n;" (make the return value the assigned value) fixes the bug, but does cause a regression: arith.sh[119]: FAIL: typecast not working in arithmetic evaluation Archaeological research shows that this test was introduced, along with the nv_getnum() call, sometime between 1995 and 1999. The following is most likely the corresponding entry in RELEASE, as the example in it closely matches the arith.sh regression test: 96-07-31 A bug in right to left arithmetic assignment for which the arithmetic expression (( y = x = 1.5 )) did not yield 1 for y when x was declared typeset -i was fixed. In other words, if x is an integer, the return value of the assignment is expected to be typecast to integer before being assigned to y. So that was the purpose of the added nv_getnum() call: it was a lazy way of doing the correct typecast for the variable's type -- unfortunately with side effects. src/cmd/ksh93/include/nval.h: - Add nv_isnum() macro determining if a shell variable has numeric type attributes, and returning their bit mask if so. src/cmd/ksh93/sh/arith.c: arith(): case ASSIGN: - Replace the offending nv_getnum() call with a simple "r = n;" by default, fixing the bug. - To avoid the regression, if nv_isnum() yields attribute bits for the variable, then do "r = n" with a direct typecast to the correct type, depending on the shell variable's attribute bits. src/cmd/ksh93/sh.1: - Rewrite the $RANDOM description to match reality. (re: af6a32d1) --- NEWS | 10 +++++++++ src/cmd/ksh93/COMPATIBILITY | 10 ++++++++- src/cmd/ksh93/include/nval.h | 7 +++++-- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh.1 | 13 ++++++++---- src/cmd/ksh93/sh/arith.c | 34 ++++++++++++++++++++++++++++-- src/cmd/ksh93/tests/arith.sh | 36 +++++++++++++++++++++++++++++++- src/cmd/ksh93/tests/variables.sh | 8 ++++++- 8 files changed, 108 insertions(+), 12 deletions(-) diff --git a/NEWS b/NEWS index 321cd8bead2f..a580d772bc5e 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,16 @@ This documents significant changes in the dev branch of ksh 93u+m. For full details, see the git log at: https://github.com/ksh93/ksh Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library. +2024-01-21: + +- A bug was fixed that caused both the 'set' and 'get'/'getn' discipline + functions of a variable to be triggered when performing an assignment in + an arithmetic expression; only the 'set' discipline is now triggered when + assigning (as was always the documented behaviour). This applies to the + shell's internal disciplines as well. For example, seeding the $RANDOM + generator via an arithmetic expression assignment now no longer takes the + first value away from the reproducible pseudorandom sequence. + 2024-01-16: - Fixed a parsing bug in array subscripts containing '=' in combination with diff --git a/src/cmd/ksh93/COMPATIBILITY b/src/cmd/ksh93/COMPATIBILITY index d2f24839a8fb..35ca6c272256 100644 --- a/src/cmd/ksh93/COMPATIBILITY +++ b/src/cmd/ksh93/COMPATIBILITY @@ -10,7 +10,7 @@ For more details, see the NEWS file and for complete details, see the git log. ____________________________________________________________________________ - ksh 93u+m/1.0.5 vs. ksh 93u+ + ksh 93u+m/1.0.9 vs. ksh 93u+ The following is a list of changes between ksh 93u+ 2012-08-01 and the new ksh 93u+m reboot that could cause incompatibilities in rare corner cases. @@ -192,17 +192,25 @@ For more details, see the NEWS file and for complete details, see the git log. 33. The -e option of test/[/[[ no longer returns true for pseudodevices that are only supported by the shell and do not in fact exist in the file system, such as /dev/tcp/*. + (Post-1.0 fix introduced in ksh 93u+m/1.0.4) 34. Single and double quotes now work like backslashes to quote '!', '^' and '-' in bracket expressions in shell glob patterns, e.g., the glob pattern ["!a-z"] now matches !, a, - or z and is no longer misparsed as [!a-z]. This restores compatibility with ksh88 and non-ksh shells. + (Post-1.0 fix introduced in ksh 93u+m/1.0.4) 35. In the ${parameter/pattern/string} search-and-replace expansion, an anchored empty pattern will now match the beginning or the end of the string, so that ${parameter/#/string} will prefix the string to the parameter value and ${parameter/%/string} will append it. Previously, these operations were no-ops (equivalent to ${parameter}). + (Post-1.0 fix introduced in ksh 93u+m/1.0.5) + +36. An assignment within an arithmetic expression no longer triggers both + of the variable's 'set' and 'get' discipline functions (if set). Only + the 'set' discipline is now triggered, as it was always documented. + (Post-1.0 fix introduced in ksh 93u+m/1.0.9) ____________________________________________________________________________ diff --git a/src/cmd/ksh93/include/nval.h b/src/cmd/ksh93/include/nval.h index adef2ae92e86..677199c608f2 100644 --- a/src/cmd/ksh93/include/nval.h +++ b/src/cmd/ksh93/include/nval.h @@ -2,7 +2,7 @@ * * * This software is part of the ast package * * Copyright (c) 1982-2012 AT&T Intellectual Property * -* Copyright (c) 2020-2022 Contributors to ksh 93u+m * +* Copyright (c) 2020-2024 Contributors to ksh 93u+m * * and is licensed under the * * Eclipse Public License, Version 2.0 * * * @@ -196,12 +196,15 @@ struct Namval #define NV_UINT16P (NV_LJUST|NV_UNSIGN|NV_SHORT|NV_INTEGER) #define NV_UINT16 (NV_UNSIGN|NV_SHORT|NV_INTEGER) #define NV_INT32 (NV_INTEGER) -#define NV_UNT32 (NV_UNSIGN|NV_INTEGER) +#define NV_UINT32 (NV_UNSIGN|NV_INTEGER) #define NV_INT64 (NV_LONG|NV_INTEGER) #define NV_UINT64 (NV_UNSIGN|NV_LONG|NV_INTEGER) #define NV_FLOAT (NV_SHORT|NV_DOUBLE) #define NV_LDOUBLE (NV_LONG|NV_DOUBLE) +/* check/isolate all the bit flags used for numeric types */ +#define nv_isnum(np) (nv_isattr(np,NV_INTEGER)?nv_isattr(np,NV_DOUBLE|NV_INTEGER|NV_LJUST|NV_LONG|NV_SHORT|NV_UNSIGN):0) + /* name-value pair macros */ #define nv_isattr(np,f) ((np)->nvflag & (f)) #define nv_onattr(n,f) ((n)->nvflag |= (f)) diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 4ae2b8511c5e..6b1fd8cf8fae 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -18,7 +18,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.1.0-alpha" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2024-01-16" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2024-01-21" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh.1 b/src/cmd/ksh93/sh.1 index b1d54c8be2a3..b7c02b6cdc43 100644 --- a/src/cmd/ksh93/sh.1 +++ b/src/cmd/ksh93/sh.1 @@ -1940,11 +1940,16 @@ command. .TP .B .SM RANDOM -Each time this variable is referenced, a random integer, -uniformly distributed between 0 and 32767, is generated. -The sequence of random numbers can be initialized by assigning -a numeric value to +Each time this variable is referenced, a pseudorandom integer is generated, +uniformly distributed between 0 and 32767 (the 16-bit unsigned integer range) +except that the same number is never repeated twice in a row. +The sequence of pseudorandom numbers is reproducible +and can be initialized to a fixed starting point by assigning +a numeric seed value to .BR RANDOM . +Each time a new shell or subshell environment is entered (see +.I Subshells\^ +above), the sequence is automatically reset to a different point. .TP .B .SM REPLY diff --git a/src/cmd/ksh93/sh/arith.c b/src/cmd/ksh93/sh/arith.c index 22c509d592a7..ba017647ba4d 100644 --- a/src/cmd/ksh93/sh/arith.c +++ b/src/cmd/ksh93/sh/arith.c @@ -2,7 +2,7 @@ * * * This software is part of the ast package * * Copyright (c) 1982-2012 AT&T Intellectual Property * -* Copyright (c) 2020-2023 Contributors to ksh 93u+m * +* Copyright (c) 2020-2024 Contributors to ksh 93u+m * * and is licensed under the * * Eclipse Public License, Version 2.0 * * * @@ -227,6 +227,7 @@ static Sfdouble_t arith(const char **ptr, struct lval *lvalue, int type, Sfdoubl case ASSIGN: { Namval_t *np; + unsigned short attr; if (lvalue->sub && lvalue->nosub > 0) /* indexed array ARITH_ASSIGNOP */ { np = (Namval_t*)lvalue->sub; /* use saved subscript reference instead of last worked value */ @@ -253,8 +254,37 @@ static Sfdouble_t arith(const char **ptr, struct lval *lvalue, int type, Sfdoubl if(lvalue->eflag) lvalue->ptr = nv_hasdisc(np,&ENUM_disc); lvalue->eflag = 0; - r=nv_getnum(np); lvalue->value = (char*)np; + /* + * The result (r) of an assignment is its value (n), cast to the type of the variable + * assigned to. We cannot simply reobtain the value with nv_getnum() to effectuate the + * typecast, because that may trigger a getnum discipline function with side effects. + * The order of checks below is essential due to how the bit masks work (see nval.h). + */ + attr = nv_isnum(np); + if(!attr || (attr & NV_LDOUBLE)==NV_LDOUBLE) /* long float */ + r = n; + else if((attr & NV_FLOAT)==NV_FLOAT) /* short float */ + r = (float)n; + else if((attr & NV_DOUBLE)==NV_DOUBLE) /* normal float */ + r = (double)n; + else if((attr & NV_UINT64)==NV_UINT64) /* long unsigned integer */ + r = (uintmax_t)n; + else if((attr & NV_UINT16)==NV_UINT16) /* short unsigned integer */ + r = (uint16_t)n; + else if((attr & NV_UINT32)==NV_UINT32) /* normal unsigned integer */ + r = (uint32_t)n; + else if((attr & NV_INT64)==NV_INT64) /* long signed integer */ + r = (intmax_t)n; + else if((attr & NV_INT16)==NV_INT16) /* short signed integer */ + r = (int16_t)n; + else if((attr & NV_INT32)==NV_INT32) /* normal signed integer */ + r = (int32_t)n; +#if _AST_release + else r = n; /* should never happen */ +#else + else abort(); +#endif break; } case LOOKUP: diff --git a/src/cmd/ksh93/tests/arith.sh b/src/cmd/ksh93/tests/arith.sh index 06773da45907..de47f55200bc 100755 --- a/src/cmd/ksh93/tests/arith.sh +++ b/src/cmd/ksh93/tests/arith.sh @@ -2,7 +2,7 @@ # # # This software is part of the ast package # # Copyright (c) 1982-2012 AT&T Intellectual Property # -# Copyright (c) 2020-2023 Contributors to ksh 93u+m # +# Copyright (c) 2020-2024 Contributors to ksh 93u+m # # and is licensed under the # # Eclipse Public License, Version 2.0 # # # @@ -995,5 +995,39 @@ function .sh.math.add x y { .sh.value=x+y; } got=$(PATH=/dev/null; typeset -i z; redirect 2>&1; z='add(2 , 3)'; echo $z) [[ e=$? -eq 0 && $got == '5' ]] || err_exit ".sh.math.* function parsing: got status $e and $(printf %q "$got")" +# ====== +# arithmetic assignments should not trigger getn disciplines, but the return +# value should still be cast to the type of the variable that is assigned to + +float x +x.getn() { .sh.value=999.99; } +let "(got = x = 1234.56) == 1234.56" || err_exit "arithmetic assignment triggers getn discipline (got $got)" +let "(got = x) == 999.99" || err_exit "arithmetic comparison fails to trigger getn discipline (got $got)" +unset x +whence -q x.getn && err_exit "unset x fails to unset -f x.getn" + +( + ulimit -c 0 # fork + Errors=0 + for sz in '' s l + do typeset -${sz}i x=0 + if ! let "(got = x = 123.95) == 123" + then err_exit "arithmetic assignment does not return properly typecast value (-${sz}i, got $got)" + fi + typeset -${sz}F x=0 + if ! let "(got = x = 123.95) == 123.95" + then err_exit "arithmetic assignment does not return properly typecast value (-${sz}F, got $got)" + fi + done +) +if let "(e = $?) > 128" +then err_exit "typeset crashed the shell (got status $e/SIG$(kill -l "$e"))" +else let "Errors += e" +fi + +if ! let "(got = RANDOM = 123.95) == 123" +then err_exit "arithmetic assignment to RANDOM does not return typecast of assigned value (got $got)" +fi + # ====== exit $((Errors<125?Errors:125)) diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index 7d2de0835886..dd6b9508a369 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/src/cmd/ksh93/tests/variables.sh @@ -2,7 +2,7 @@ # # # This software is part of the ast package # # Copyright (c) 1982-2012 AT&T Intellectual Property # -# Copyright (c) 2020-2023 Contributors to ksh 93u+m # +# Copyright (c) 2020-2024 Contributors to ksh 93u+m # # and is licensed under the # # Eclipse Public License, Version 2.0 # # # @@ -125,6 +125,12 @@ got=$(RANDOM=1; print $RANDOM; :& print $RANDOM) [[ $got == "$exp" ]] || err_exit "Background job influences reproducible $RANDOM sequence" \ "(expected $(printf %q "$exp"), got $(printf %q "$got"))" +# Seeding with an arithmetic expression should be identical to seeding using a shell assignment +RANDOM=12345; exp="$RANDOM $RANDOM $RANDOM $RANDOM" +let "RANDOM = 12345"; got="$RANDOM $RANDOM $RANDOM $RANDOM" +[[ $got == "$exp" ]] || err_exit "Seeding RANDOM using arithmetic expression fails" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + # SECONDS float secElapsed=0.0 secSleep=0.001 let SECONDS=$secElapsed