Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes caused by 'typeset -RF' #47

Merged
merged 1 commit into from
Jun 28, 2020

Conversation

JohnoKing
Copy link

@JohnoKing JohnoKing commented Jun 27, 2020

Variables created with typeset -RF were being treated as short integers, even though they are actually floating point values. As a result the following example will cause a crash (foo cannot be zero):

$ typeset -RF foo=1
$ test "$foo"

This is fixed by checking for NV_DOUBLE with nv_isattr, which prevents ksh from treating floating point values as short integers due to == NV_INT16P excluding NV_DOUBLE. This bugfix was backported from ksh93v- 2013-10-10-alpha.

Variables created with 'typeset -RF' were being treated as
short integers, even though they are actually floating point
values. As a result the following example will cause a crash:

$ typeset -RF foo=1
$ test "$foo"

This is fixed by checking for 'NV_DOUBLE' with 'nv_isattr',
which prevents ksh from treating floating point values as
short integers due to '== NV_INT16P' excluding 'NV_DOUBLE'.
This bugfix was backported from ksh93v- 2013-10-10-alpha.

src/cmd/ksh93/sh/array.c,
src/cmd/ksh93/sh/name.c,
src/cmd/ksh93/sh/nvdisc:
- Avoid treating floating point values as short integers by
  checking for 'NV_DOUBLE' with 'nv_isattr'.

src/cmd/ksh93/tests/types.sh:
- Add a regression test for the 'typeset -RF' crash. The
  crash cannot be replicated if 'typeset -RF' sets 'foo'
  to zero.
@hyenias
Copy link

hyenias commented Jun 28, 2020

How does this fix address the following restrictions as indicated from typeset --man? I think you can apply one of the justification options to the variable after but you should not be able to set them initially with its creation. I believe the L, R, and Z options are either ignored or the statement errors depending on which numeric option given.

Not all option combinations are possible. For example, the numeric options
-i, -E, and -F cannot be specified with the justification options -L, -R, and -Z.

@JohnoKing
Copy link
Author

This bugfix causes the right justify option to be ignored (rather than crash the shell):

$ typeset -R3F foo=32.01
$ echo $foo
32.0100000000

@McDutchie
Copy link

McDutchie commented Jun 28, 2020

Hmm. typeset -R10 -i foo=1; echo "[$foo]" gives a usage error and something like typeset -R10 -E3 foo=12345; echo "[$foo]" simply ignores the -R flag.

Which precedent should we follow? Maybe they should all error out? I think giving a usage error is more logical since this combination of flags is not actually possible. But it should be preceded by an informative error message such as "incompatible options combination".

Or maybe we should just fix it so they can be combined? I see no real reason why this should be impossible. On mksh (which only supports integer arithmetic) the following works fine:

$ mksh -c 'typeset -R10 -i foo=123 bar=4567; printf "%s\n" "[$foo]" "[$bar]"'
[       123]
[      4567]

The other ksh-inspired shell that supports floating point arithmetic is zsh, and it has no problem combining any of these:

$ zsh -c 'typeset -R20 -F foo=12345; echo "[$foo]"'
[    12345.0000000000]
$ zsh -c 'typeset -R20 -E3 foo=12345; echo "[$foo]"'
[            1.23e+04]
$ zsh -c 'typeset -R20 -i foo=12345; echo "[$foo]"'
[               12345]

Whichever we decide to do, applying the current patch seems like a good idea regardless, as it makes the code more crash-proof at no expense.

@McDutchie McDutchie merged commit 5135cf6 into ksh93:master Jun 28, 2020
@hyenias
Copy link

hyenias commented Jun 28, 2020

I think having typeset being able to recreate a variable completely in one line is the correct thing to do. I should be able to do a typeset -p on a variable and then utilize that output to recreate it. I would differently like to have all the options supported consistently for the numeric types if appropriate. But that is out-of-scope for this particular bugfix. I do like the usage error being given if that could be done consistently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants