Skip to content

Commit

Permalink
arithmetic: Fix even more octal leading zero mess (re: 5da8eb3)
Browse files Browse the repository at this point in the history
We still weren't done. <sigh>

Bug 1: leading-zero octal numbers with invalid digits 8 or 9 in
them are reinterpreted as decimal, which is flagrantly incorrect
behaviour. This MUST throw an error because it is a fatal user
error in the script. This bug has always been in ksh, but became
mostly hidden since the referenced commit, now triggered only in
the 'let' builtin with --letoctal on, or globally with --posix on.
The following example makes it easy to see how badly wrong this is:
  $ set --posix
  $ echo $((0125)) $((0126)) $((0127)) $((0128)) $((0129))
  85 86 87 128 129
Absolutely every other shell (provided leading-zero octal is
activated) throws an error at 0128, as they should.

Bug 2: in arithmetic expansions, the '0x' prefix for hexadecimal
numbers, and ksh's n# prefix for base n (e.g., 16# for hex nums),
incorrectly accept leading zeros.
  # on 93u+m after the referenced commit (arith error on 93u+)
  $ echo $((00x1A)) $((016#1A))
  26 26
  # on 93u+ as well as 93u+m
  $ let "x=00x1A" "y=016#1A"; echo $x $y
  26 26
Expected: arithmetic syntax error in both cases.
Note: The --posix option (on 93u+m), or the --letoctal option for
the let builtin (on 93u+ and 93u+m), disables this bug so that the
expected error is thrown. This means the bug is caused by incorrect
disabling of initial zeros as an octal prefix. All initial zeros
followed by a digit are simply discarded unconditionally.

src/cmd/ksh93/sh/arith.c: arith(): case LOOKUP:
- Delete incorrect unconditional skipping of initial zeros.
- Delete check for '8' and '9' digits for reparsing as a decimal
  number upon error from libast's strtonll() function.
- Instead, always reparse as decimal if lastbase is 8 (which means
  strtonll() detected an octal number, correct or otherwise), AND
  the number string starts with 0, AND the --letoctal or --posix
  option (as applicable) is active.

The above *should* have cleanly fixed both bug 1 and bug 2, but a
mystifying problem remains. Bug 1 is left, but *only* for the digit
immediately following the leading zero:
  $ set --posix
  $ echo $((09))
  9
  $ echo $((019))
  ksh: 019: arithmetic syntax error
  $ echo $((0119))
  ksh: 0119: arithmetic syntax error
The only way in which this could possibly make sense is if libast's
strtonll() has a bug as well. So, we have to dig into that, too.

In src/lib/libast/string/strtonll.c, strtonll() is implemented by
defining a few macros and then including strtoi.h, which has the
common code for the strto* functions. So the bug must be there.

Sure enough, when searching for 'base = 8;' to find where octal is
set, the problem quickly becomes apparent. The following code is
executed when 'base' (which is 'lastbase' in ksh) is initially 0:

267:	else if (S2I_valid(s) && *s == '0' && S2I_valid(s + 1))
268:	{
269:		if ((c = *(s + 1)) == 'x' || c == 'X') // '0x': hex
[...]
274:		else if (c >= '0' && c <= '7')   // <<<=== ah ha!!!
275:		{
276:			s++;
277:			base = 8;
278:		}
279:	}

That's right, the base 8 is only activated if the first digit
following the leading 0 is a valid octal digit. Otherwise, 'base'
stays 0 and thus defaults to 10 on lines 281-282, causing the bug.

src/lib/libast/string/strtoi.h:
- Delete incorrect check for initial octal digit. When 'base' is
  initially 0, it is now always set to 8 when there is a leading
  zero in the number string (unless followed by 'x' or 'X').

This fixes the rest of bug 1. And now, hopefully this is finally
the end of the octal leading-zero mess.
  • Loading branch information
McDutchie committed Jul 17, 2024
1 parent f1e5a19 commit d2c4bf5
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 11 deletions.
11 changes: 11 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,17 @@ 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-07-17:

- Fixed: if the --posix or --letoctal option was turned on, octal numbers
designated by the leading-zero octal prefix enabled by these options but
containing the invalid digits 8 or 9 (e.g., $((09)) or let "09") failed
to throw an error and were instead incorrectly reinterpreted as decimal.

- Fixed: invalid leading zeros in the '0x' hexadecimal prefix or in ksh
arithmetic base specifiers, e.g., 00x2F or 016#2F (instead of 0x2F or 16#2F),
failed to throw an arithmetic syntax error.

2024-07-16:

- The history expansion character ('!' by default) is now not processed when
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -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-07-16" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-07-17" /* 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. */
Expand Down
9 changes: 2 additions & 7 deletions src/cmd/ksh93/sh/arith.c
Original file line number Diff line number Diff line change
Expand Up @@ -435,15 +435,10 @@ static Sfdouble_t arith(const char **ptr, struct lval *lvalue, int type, Sfdoubl
char lastbase=0, *val = xp, oerrno = errno;
lvalue->eflag = 0;
errno = 0;
if(!sh_isoption(sh.bltinfun==b_let ? SH_LETOCTAL : SH_POSIX))
{
/* Skip leading zeros to avoid parsing as octal */
while(*val=='0' && isdigit(val[1]))
val++;
}
r = strtonll(val,&str, &lastbase,-1);
if(*str=='8' || *str=='9')
if(lastbase==8 && *val=='0' && !sh_isoption(sh.bltinfun==b_let ? SH_LETOCTAL : SH_POSIX))
{
/* disable leading-0 octal by reparsing as decimal */
lastbase=10;
errno = 0;
r = strtonll(val,&str, &lastbase,-1);
Expand Down
37 changes: 36 additions & 1 deletion src/cmd/ksh93/tests/arith.sh
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ unset x
let x=010
[[ $x == 10 ]] || err_exit 'let treating 010 as octal'
(set -o letoctal; let x=010; [[ $x == 8 ]]) || err_exit 'let not treating 010 as octal with letoctal on'
if [[ -o ?posix ]]
if ((HAVE_posix))
then (set -o posix; let x=010; [[ $x == 8 ]]) || err_exit 'let not treating 010 as octal with posix on'
fi

Expand Down Expand Up @@ -1008,13 +1008,24 @@ got=$(PATH=/dev/null; typeset -i z; set +x; redirect 2>&1; z='add(2 , 3)'; echo
# 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

(
ulimit -c 0 # fork
Errors=0
# <<< outdent
float x
x.getn() { .sh.value=987.65; }
let "got = x = 1234.56"
[[ $got == 1234.56* ]] || err_exit "arithmetic assignment triggers getn discipline (got $got)"
[[ $x == 987.65* ]] || err_exit "arithmetic comparison fails to trigger getn discipline (got $x)"
unset x
whence -q x.getn && err_exit "unset x fails to unset -f x.getn"
# >>> indent
exit $Errors
)
if let "(e = $?) > 128"
then err_exit "getn discipline crashed the shell (got status $e/SIG$(kill -l "$e"))"
else let "Errors += e"
fi

(
ulimit -c 0 # fork
Expand All @@ -1030,6 +1041,7 @@ whence -q x.getn && err_exit "unset x fails to unset -f x.getn"
then err_exit "arithmetic assignment does not return properly typecast value (-${sz}F, got $got)"
fi
done
exit $Errors
)
if let "(e = $?) > 128"
then err_exit "typeset crashed the shell (got status $e/SIG$(kill -l "$e"))"
Expand Down Expand Up @@ -1058,5 +1070,28 @@ integer 20 got=$exp
[[ $got == "$exp" ]] || err_exit "negative base-20 number (expected '$exp', got '$got')"
unset got

# ======
exp=': arithmetic syntax error'
# when leading 0 is recognised as octal, invalid octal should be an error
for v in 08 028 089 09 029 098 012345678
do got=$(set --letoctal; let "$v" 2>&1)
[[ e=$? -eq 1 && $got == *"$exp" ]] || err_exit "invalid leading-zero octal number $v not an error" \
"(expected status 1 and match of *'$exp', got status $e and '$got')"
done
# only a single 0 should precede x or X for it to be a hexadecimal number
got=$(eval ': $((00xF))' 2>&1)
[[ e=$? -eq 1 && $got == *"$exp" ]] || err_exit "extra zero before 0x not an error (expansion)" \
"(expected status 1 and match of *'$exp', got status $e and '$got')"
got=$(let '00xF' 2>&1)
[[ e=$? -eq 1 && $got == *"$exp" ]] || err_exit "extra zero before 0x not an error (let)" \
"(expected status 1 and match of *'$exp', got status $e and '$got')"
# ksh base specifiers (like 16# for hexadecimal) may not have leading zeros
got=$(eval ': $((016#F))' 2>&1)
[[ e=$? -eq 1 && $got == *"$exp" ]] || err_exit "leading zero before 16# not an error (expansion)" \
"(expected status 1 and match of *'$exp', got status $e and '$got')"
got=$(let '016#F' 2>&1)
[[ e=$? -eq 1 && $got == *"$exp" ]] || err_exit "leading zero before 16# not an error (let)" \
"(expected status 1 and match of *'$exp', got status $e and '$got')"

# ======
exit $((Errors<125?Errors:125))
7 changes: 7 additions & 0 deletions src/cmd/ksh93/tests/posix.sh
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ test 010 -eq 10 || err_exit "'test' not ignoring leading octal zero in --posix"
[[ 010 -eq 8 ]] || err_exit "'[[' ignoring leading octal zero in --posix"
(set --noposix; [[ 010 -eq 10 ]]) || err_exit "'[[' not ignoring leading octal zero in --noposix"
exp=': arithmetic syntax error'
for v in 08 028 089 09 029 098 012345678
do got=$(eval ": \$(($v))" 2>&1)
[[ e=$? -eq 1 && $got == *"$exp" ]] || err_exit "invalid leading-zero octal number $v not an error" \
"(expected status 1 and match of *'$exp', got status $e and '$got')"
done
# disables zero-padding of seconds in the output of the time and times built-ins;
case ${.sh.version} in
*93u+m/1.0.*) exp=$'^user\t0m0.[0-9]{2}s\nsys\t0m0.[0-9]{2}s\n0m0.[0-9]{3}s 0m0.[0-9]{3}s\n0m0.000s 0m0.000s$' ;;
Expand Down
4 changes: 2 additions & 2 deletions src/lib/libast/string/strtoi.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
* base: nnn# base nnn
* 0[xX] hex
* 0 octal
* [1-9] decimal
* (omitted) decimal
*
* number: [0-9a-zA-Z]*
*
Expand Down Expand Up @@ -271,7 +271,7 @@ S2I_function(const char* a, char** e, int base)
k = s += 2;
base = 16;
}
else if (c >= '0' && c <= '7')
else
{
s++;
base = 8;
Expand Down

0 comments on commit d2c4bf5

Please sign in to comment.