landley / toybox

toybox
http://landley.net/toybox
BSD Zero Clause License
2.44k stars 340 forks source link

POSIX deviation: test arithmetic operands must support decimal only #498

Open stephane-chazelas opened 7 months ago

stephane-chazelas commented 7 months ago
toybox test 10 -eq 010

returns false and

toybox test 08 -lt 09

gives an error.

POSIX does require operands to be treated as decimal which means it's an undocumented deviation.

The fact that [ arithmetic operators accept only decimal in bash/zsh while [[ ... ]] accepts any arithmetic expression (including octal constants if not disabled globally), is a reason why [ is often recommended over [[ or (( in those shells (in particular [[ $attacker_controlled_variable -lt 10 ]] and (( attacker_controlled_variable < 10 )) are command injection vulnerabilities there).

It's useful to be able to rely on decimal handling when handling 0-padded number such as date/time components as day, month, hour, minute, second are usually 0-padded as in:

if [ "$(date +%m)" -le 6 ]; then
  echo first semester
fi

(toybox date supports date +%-m but that's not standard/portable)

If you decide to keep the POSIX deviation, I would suggest that at least sequences of digits that contain 8 and 9 digits be treated as decimal (both 010 and 08 to be considered as number 8) so we can still do date manipulation without having to strip the leading 0s manually.

POSIX doesn't specify the behaviour for test 0xf -eq 15 or test 1c -eq 1 or test 1000 -eq 1kd so it's fine in that regard to return true instead of an error.

oliverkwebb commented 7 months ago
toybox test 10 -eq 010

returns false and

toybox test 08 -lt 09

Still better than GNU behavior...

$ /bin/test 08 -lt 09; echo $?
0
$ toybox test 08 -lt 09; echo $?
test: not integer: 08
2

08 isn't valid octal. So what exactly is GNU test being accurate to?

POSIX does require operands to be treated as decimal which means it's an undocumented deviation.

The fact that [ arithmetic operators accept only decimal in bash/zsh while [[ ... ]] accepts any arithmetic expression (including octal constants if not disabled globally), is a reason why [ is often recommended over [[ or (( in those shells (in particular [[ $attacker_controlled_variable -lt 10 ]] and (( attacker_controlled_variable < 10 )) are command injection vulnerabilities there).

Toybox aliases [ to [[, so they are both functionally the same thing.

Another cool thing you can do by parsing numbers with atolx: test 100M -lt 10G

It's useful to be able to rely on decimal handling when handling 0-padded number such as date/time components as day, month, hour, minute, second are usually 0-padded as in:

if [ "$(date +%m)" -le 6 ]; then
  echo first semester
fi

(toybox date supports date +%-m but that's not standard/portable)

Not portable to what? GNU and busybox have it, FreeBSD also has it and therefore MacOS does too, but admittedly OpenBSD doesn't looking at their strftime manpage. Which means we are not portable to a OpenBSD environment where toybox test is being used, but toybox date isn't.

A quick hack to fix this is to swap out atolx() with regular atol() (Line 82 toys/posix/test.c is where the magic happens). But to keep true POSIX and Bash compatibility there would need to be some way to detect if we are calling under "[[", and only do the octal numbers and such if so.

The patch is on the list, but I have my doubts on Rob applying it due to the loss of functionality.

If you ever are working on a OpenBSD box and trying to interoperate with zero padded month numbers that you can't turn off with OpenBSD strftime, but can use in toybox test. And it's really super security critical (How exactly is a system supposed to be attacked with this minor bug? Then again I could say that about most of the modern CVE's bugs), but also speed critical enough to not pipe the numbers through grep -o "[1-9][0-9]*" to remove the trailing 0's. I'd suggest applying it yourself

stephane-chazelas commented 7 months ago

Still better than GNU behavior...

$ /bin/test 08 -lt 09; echo $?
0
$ toybox test 08 -lt 09; echo $?
test: not integer: 08
2

08 isn't valid octal. So what exactly is GNU test being accurate to?

That's the whole point of this issue. 08 and 09 are also decimal numbers and must be interpreted as such, not octal.

That's not just GNU test. That's virtually all test implementations in existence including the test builtin of most shells including mksh.

POSIX does require operands to be treated as decimal which means it's an undocumented deviation. The fact that [ arithmetic operators accept only decimal in bash/zsh while [[ ... ]] accepts any arithmetic expression (including octal constants if not disabled globally), is a reason why [ is often recommended over [[ or (( in those shells (in particular [[ $attacker_controlled_variable -lt 10 ]] and (( attacker_controlled_variable < 10 )) are command injection vulnerabilities there).

Toybox aliases [ to [[, so they are both functionally the same thing.

Note that [[...]] is initially a shell construct from the Korn shell inside which a separate micro-language is recognised to avoid the shortcomings of the [ utility. It's not meant to be implemented as a standalone utility.

Another cool thing you can do by parsing numbers with atolx: test 100M -lt 10G

As I mentioned in my initial report, it's fine to keep that as that's outside POSIX scope (behaviour unspecified).

Not portable to what? GNU and busybox have it, FreeBSD also has it and therefore MacOS does too, but admittedly OpenBSD doesn't looking at their strftime manpage. Which means we are not portable to a OpenBSD environment where toybox test is being used, but toybox date isn't. [...]

My view is rather that a portable script will use [ "$(date +%m)" -le 6 ] as that's standard and guaranteed to work by POSIX, and using a $(date +%-m) work around should not be necessary and is not even standard.

oliverkwebb commented 7 months ago

Note that [[...]] is initially a shell construct from the Korn shell inside which a separate micro-language is recognised to avoid the shortcomings of the [ utility. It's not meant to be implemented as a standalone utility.

We can do both since one is a complete (Apparently, mostly complete) subset of the other.

Another cool thing you can do by parsing numbers with atolx: test 100M -lt 10G

As I mentioned in my initial report, it's fine to keep that as that's outside POSIX scope (behaviour unspecified).

The problem is that there is no easy way from my knowledge to keep one and not the other, I may be wrong though.

My view is rather that a portable script will use [ "$(date +%m)" -le 6 ] as that's standard and guaranteed to work by POSIX, and using a $(date +%-m) work around should not be necessary and is not even standard.

Like I said, you can pipe numbers through grep -o "[1-9][0-9]*" to strip the trailing zeros

stephane-chazelas commented 7 months ago

FWIW, date +%-m works in neither OpenBSD nor NetBSD.

Note that [[...]] is initially a shell construct from the Korn shell inside which a separate micro-language is recognised to avoid the shortcomings of the [ utility. It's not meant to be implemented as a standalone utility.

We can do both since one is a complete (Apparently, mostly complete) subset of the other.

They have incompatible syntax and feature set in all shells that have both.

[...] The problem is that there is no easy way from my knowledge to keep one and not the other, I may be wrong though.

It would be enough to do:

if (operand-has-only-digits[-and-whitespace])
  convert-from-decimal;
else
  use-atolx;

I would think (the and-whitespace is not a POSIX requirement but is a nice to have as most test implementations handle leading and trailing whitespace).

My view is rather that a portable script will use [ "$(date +%m)" -le 6 ] as that's standard and guaranteed to work by POSIX, and using a $(date +%-m) work around should not be necessary and is not even standard.

Like I said, you can pipe numbers through grep -o "[1-9][0-9]*" to strip the trailing zeros

grep -o is a non-standard GNUism, and that fails for 00.

You need something like:

hour=$(date +%H)
short_hour=${hour#0}
if [ "$short_hour" -lt 12 ]; then
  echo "$hour is before noon.
fi
stephane-chazelas commented 7 months ago

Re: your patch

Note that there's no security issue in toybox' test (that I know). I mentioned zsh/bash's security issue with their own [[...]] construct (not [[ utility, they don't have a [[ builtin) which treat operands as arithmetic expressions as a note to stress how we usually rely on [ doing only decimal parsing for its arithmetic operators in those shells.

oliverkwebb commented 7 months ago
if (operand-has-only-digits[-and-whitespace])
  convert-from-decimal;
else
  use-atolx;

"if (operand-has-only-digits[-and-whitespace])" is a oversimplification of what that code would actually look like, you'd need a for loop going over each element and checking it every time you went over a number. Which adds a lot of code and slows this down by a fair amount. You could check the first character and if that's a zero don't process the octal, but that removes hexadecimal support.

This is something that can be done, but it adds complexity, adds executable size, and slows the code down, and gives using function pointers will warnings because of mismatches between a "char " and a "const char " (sigh).

I may be wrong, and the support for prefixes and hex numbers in test is actually something less important than I thought. It's not my decision to make

Maybe there should be some way to turn off octal support for atolx. I just found out that there isn't prefix support in expr, and doing something like expr 100M + 64k + ... to automatically get the number of bytes would be useful. But adding in octal support would bring this problem.

atolx is used in commands like head and tail too, I originally thought it was how lib/args.c parsed numbers too. So this problem isn't just in test, it's a large number of places.

Octal is obsolete almost everywhere except file permissions. I'd suggest removing it from atolx entirely, but doing that has the same problems of detecting a octal number and parsing it as decimal, but not a hexadecimal one

And of course, similar discussion about this problem for dd is on the list:

I noticed a problem with numeric arguments to dd e.g. ibs=999, skip=1234 or count=0000987

The 0.6.1 implementation assumes that the number is decimal.

0.7.5 assumes that a leading 0 indicates octal, this is different to both the posix standard and the implementation in standard linux.

Indeed. I had common code to handle the suffixes but it also does the c-standard base detection. Which is a side effect you don't want. Hmmm...

On Posix Being the be-all, end-all of implementation details:

The dd on ubuntu and as specified in the posix spec (http://pubs.opengroup.org/onlinepubs/9699919799/) says the numbers are decimal

The posix spec says that the command supports ebcdic to ascii conversions. I've been taking a somewhat loose interpretation of this one. :)

Also:

("This broke my script" is a different issue from "the spec says you should do this even though nobody seems to have used it in living memory".)

(This is something I was considering suggesting):

I think that the problem is caused by the use of atolx() as a common numeric input function for many dissimilar command line utilities.

Maybe an extra param is needed to indicate the expected base, e.g. like the base param to strtol.

I'm reluctant to add an argument to a library function that's currently used 57 times without it. If the problem is just leading zeroes, a trivial wrapper to "while (*str=='0') str++;" in this command should fix it?

And Finally:

I agree that octal is obsolete (outside of file permissions), and if posix says it's decimal then leading zeroes changing that is surprising and should probably be fixed. I'm less convinced about the x thing (which means not supporting hexadecimal, which is not obsolete...)

oliverkwebb commented 7 months ago

After more consideration, removing octal support in atolx seems like a better solution. The use of zero padded numbers probably vastly outweighs any modern use of Octal, so confusing one for another isn't that good of a idea.

Patch 2.0 is on the list

stephane-chazelas commented 7 months ago

After more consideration, removing octal support in atolx seems like a better solution. The use of zero padded numbers probably vastly outweighs any modern use of Octal, so confusing one for another isn't that good of a idea.

Seems sensible to me. Those 0123 treated as octal have been a plague since forever. The 8#123 of ksh or 0o123 found in some languages are arguably much better.

Those 0123 treated as octal have already been removed from arithmetic expressions of some shells including mksh and ksh93 (and defunct ksh2020) when not in posix mode (ksh being the shell that introduced them in the first place). zsh never treated those as octal (though added a octalzeroes option in 3.1.7 in 2000 for its sh emulation mode).

landley commented 7 months ago

I'm always a little confused about the "do less" reports. I have a pending one in sed on my todo list too.

I can see leading zeroes indicating octal being considered confusing to a modern audience, now that 6 bit systems are obsolete and the only fossil use for it that comes to mind is unix permissions. That said, strtol() detects that when it detects 0x and that's libc plumbing, I didn't write that, just used it.

Haven't decided what to do about it yet, the reporter of the sed one hadn't actually hit a case where it caused a problem, they were just checking compliance corner cases. Did you have a use case here, or were you just doing compliance checking too?

stephane-chazelas commented 7 months ago

I'm always a little confused about the "do less" reports. I have a pending one in sed on my todo list too.

I can see leading zeroes indicating octal being considered confusing to a modern audience, now that 6 bit systems are obsolete and the only fossil use for it that comes to mind is unix permissions. That said, strtol() detects that when it detects 0x and that's libc plumbing, I didn't write that, just used it.

Yes, strtol() is meant to handle 0123 as octal and that should remain as so. If atolx() is used in places where strtol() is meant to be used like in printf %d 0123, then 0123 handling should not be removed from there as that would break scripts that expect that octal handling.

Haven't decided what to do about it yet, the reporter of the sed one hadn't actually hit a case where it caused a problem, they were just checking compliance corner cases. Did you have a use case here, or were you just doing compliance checking too?

I'm not a (direct) user of toybox myself. I came across it whilst researching for this answer to a question about find -size (where handling 0123 as octal is also a non-conformance but likely less of a problem there as 0-padded decimals are less likely).

Having said that, questions about why (( zero_padded_decimal < 10 )) or [[ zero_padded_decimal -lt 10 ]] don't work properly in bash (one of the Korn-like shells that haven't removed 0123 handling as octal yet in non-sh mode) are common on usenet or stackexchange (and zero-padded numbers are common in date/time components at least) and the usual answer in that particular case is: use [ instead which POSIX guarantees operands to arithmetic operators to be treated as literal (and is standard as opposed to ((...)), [[...]] so can be used in sh scripts).

In the general case, in Korn-like shells 10#$zero_padded_decimal in arithmetic expressions works but not for negative numbers.

oliverkwebb commented 7 months ago

Yes, strtol() is meant to handle 0123 as octal and that should remain as so. If atolx() is used in places where strtol() is meant to be used like in printf %d 0123, then 0123 handling should not be removed from there as that would break scripts that expect that octal handling.

GNU printf doesn't do octal, although busybox does:

$ for i in printf 'busybox printf' 'toybox printf'; do $i "%d\n" 0123; done
123
83
83
stephane-chazelas commented 7 months ago

GNU printf doesn't do octal, although busybox does:

$ for i in printf 'busybox printf' 'toybox printf'; do $i "%d\n" 0123; done
123
83
83

Note that there are two GNU implementations of the standard printf utility, the standalone printf executable from GNU coreutils and the printf builtin of the GNU shell (aka bash).

I find that both do octal here on Debian with GNU coreutils 9.4 and bash 5.2. According to ltrace, GNU coreutils printf defers the conversion from text to integer to strtoimax() which is meant to be like strtol() except it converts to intmax_t instead of long.

Most shells have the printf utility builtin, so it could very well be that it's the builtin of your shell that doesn't treat 0123 as octal. Recent versions of ksh93 are known not to when their posix option is not enabled (running it as sh sets the posix option).