zsh-workers
 help / color / mirror / code / Atom feed
* I think I found a serious bug in zstrtoul_underscore
@ 2022-11-05 21:57 Clinton Bunch
  2022-11-05 22:13 ` Clinton Bunch
  2022-11-06  8:43 ` Roman Perepelitsa
  0 siblings, 2 replies; 7+ messages in thread
From: Clinton Bunch @ 2022-11-05 21:57 UTC (permalink / raw)
  To: zsh-workers

This is how I'm calling it:

if (!zstrtoul_underscore(OPT_ARG(ops, 'L'), &lower)) {

lower is declared as a zulong.

When fed a negative number it seg faults, on Rocky Linux 8 at least.

Example:

helikon[~/tmp/zsh]% getrandom -L 0 -U $(( 2**32-1))
Test before convert -L
Test after convert -L
377689410
helikon[~/tmp/zsh]% getrandom -L -1 -U 5
Test before convert -L
zsh: segmentation fault (core dumped)  ~/local/bin/zsh



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: I think I found a serious bug in zstrtoul_underscore
  2022-11-05 21:57 I think I found a serious bug in zstrtoul_underscore Clinton Bunch
@ 2022-11-05 22:13 ` Clinton Bunch
  2022-11-05 23:41   ` Clinton Bunch
  2022-11-06  8:43 ` Roman Perepelitsa
  1 sibling, 1 reply; 7+ messages in thread
From: Clinton Bunch @ 2022-11-05 22:13 UTC (permalink / raw)
  To: zsh-workers

On 11/5/2022 4:57 PM, Clinton Bunch wrote:
> This is how I'm calling it:
>
> if (!zstrtoul_underscore(OPT_ARG(ops, 'L'), &lower)) {
>
> lower is declared as a zulong.
>
> When fed a negative number it seg faults, on Rocky Linux 8 at least.
>
> Example:
>
> helikon[~/tmp/zsh]% getrandom -L 0 -U $(( 2**32-1))
> Test before convert -L
> Test after convert -L
> 377689410
> helikon[~/tmp/zsh]% getrandom -L -1 -U 5
> Test before convert -L
> zsh: segmentation fault (core dumped)  ~/local/bin/zsh
>
>
I've verified this on FreeBSD 13.1 and Solaris 11.4 as well, so unless 
I'm calling it wrong, this seems like a serious bug.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: I think I found a serious bug in zstrtoul_underscore
  2022-11-05 22:13 ` Clinton Bunch
@ 2022-11-05 23:41   ` Clinton Bunch
  2022-11-06  0:24     ` [PATCH} " Clinton Bunch
  0 siblings, 1 reply; 7+ messages in thread
From: Clinton Bunch @ 2022-11-05 23:41 UTC (permalink / raw)
  To: zsh-workers

On 11/5/2022 5:13 PM, Clinton Bunch wrote:
> On 11/5/2022 4:57 PM, Clinton Bunch wrote:
>> This is how I'm calling it:
>>
>> if (!zstrtoul_underscore(OPT_ARG(ops, 'L'), &lower)) {
>>
> I've verified this on FreeBSD 13.1 and Solaris 11.4 as well, so unless 
> I'm calling it wrong, this seems like a serious bug.
>
>
Apparently the problem is with argument handling.  Zsh appears to 
interpret a negative number as another option (but doesn't error with 
unknown option) but  I get the same seg-fault when I don't specify an 
argument to -L as when I specify a negative number.

OPT_ARG_SAFE seems to prevent the seg-fault at that point, but I still 
get one in zstrtoul_underscore.  It seems like specifying that -L 
accepts an argument (specifically a number) should cause it to throw an 
error at argument parse time rather than at recovering the argument.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH} Re: I think I found a serious bug in zstrtoul_underscore
  2022-11-05 23:41   ` Clinton Bunch
@ 2022-11-06  0:24     ` Clinton Bunch
  2022-11-06  5:16       ` Mikael Magnusson
  0 siblings, 1 reply; 7+ messages in thread
From: Clinton Bunch @ 2022-11-06  0:24 UTC (permalink / raw)
  To: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On 11/5/2022 6:41 PM, Clinton Bunch wrote:
> On 11/5/2022 5:13 PM, Clinton Bunch wrote:
>> On 11/5/2022 4:57 PM, Clinton Bunch wrote:
>>> This is how I'm calling it:
>>>
>>> if (!zstrtoul_underscore(OPT_ARG(ops, 'L'), &lower)) {
>>>
>> I've verified this on FreeBSD 13.1 and Solaris 11.4 as well, so 
>> unless I'm calling it wrong, this seems like a serious bug.
>>
>>
> Apparently the problem is with argument handling.  Zsh appears to 
> interpret a negative number as another option (but doesn't error with 
> unknown option) but  I get the same seg-fault when I don't specify an 
> argument to -L as when I specify a negative number.
>
> OPT_ARG_SAFE seems to prevent the seg-fault at that point, but I still 
> get one in zstrtoul_underscore.  It seems like specifying that -L 
> accepts an argument (specifically a number) should cause it to throw 
> an error at argument parse time rather than at recovering the argument.
>
>
I see now my mistake in the options segment of bintab, ":%" specifies an 
*optional* numeric argument.   Removing the % makes it fail correctly.  
Which leads to the question why would there be an optional numeric 
argument specification, but not a mandatory numeric argument specification?

Though it did uncover a pathological case that caused a catastrophic 
failure in zstrtoul_underscore.  It doesn't check that s is null before 
it starts trying to convert.  I've attached the trivial patch.

[-- Attachment #2: zstrtoul_underscore-NULL.patch --]
[-- Type: text/plain, Size: 284 bytes --]

diff --git a/Src/utils.c b/Src/utils.c
index edf5d3df7..ede7b7af6 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2481,6 +2481,9 @@ zstrtoul_underscore(const char *s, zulong *retval)
 {
     zulong calc = 0, newcalc = 0, base;
 
+    if (!s)
+	return 0;
+
     if (*s == '+')
 	s++;
 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH} Re: I think I found a serious bug in zstrtoul_underscore
  2022-11-06  0:24     ` [PATCH} " Clinton Bunch
@ 2022-11-06  5:16       ` Mikael Magnusson
  0 siblings, 0 replies; 7+ messages in thread
From: Mikael Magnusson @ 2022-11-06  5:16 UTC (permalink / raw)
  To: Clinton Bunch; +Cc: zsh-workers

On 11/6/22, Clinton Bunch <cdbunch@zentaur.org> wrote:

> I see now my mistake in the options segment of bintab, ":%" specifies an
> *optional* numeric argument.   Removing the % makes it fail correctly.
> Which leads to the question why would there be an optional numeric
> argument specification, but not a mandatory numeric argument specification?
>
> Though it did uncover a pathological case that caused a catastrophic
> failure in zstrtoul_underscore.  It doesn't check that s is null before
> it starts trying to convert.  I've attached the trivial patch.

If we did this, it would make more sense to add the check in
zstrtol_underscore which has more than one current user, however it is
expected that functions like this crash on NULL, the standard library
strtol (and most other str*) does this as well.

-- 
Mikael Magnusson


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: I think I found a serious bug in zstrtoul_underscore
  2022-11-05 21:57 I think I found a serious bug in zstrtoul_underscore Clinton Bunch
  2022-11-05 22:13 ` Clinton Bunch
@ 2022-11-06  8:43 ` Roman Perepelitsa
  2022-11-06 10:30   ` Clinton Bunch
  1 sibling, 1 reply; 7+ messages in thread
From: Roman Perepelitsa @ 2022-11-06  8:43 UTC (permalink / raw)
  To: Clinton Bunch; +Cc: zsh-workers

On Sat, Nov 5, 2022 at 10:58 PM Clinton Bunch <cdb_zsh@zentaur.org> wrote:
>
>
> helikon[~/tmp/zsh]% getrandom -L 0 -U $(( 2**32-1))

What is getrandom?

Roman.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: I think I found a serious bug in zstrtoul_underscore
  2022-11-06  8:43 ` Roman Perepelitsa
@ 2022-11-06 10:30   ` Clinton Bunch
  0 siblings, 0 replies; 7+ messages in thread
From: Clinton Bunch @ 2022-11-06 10:30 UTC (permalink / raw)
  To: zsh-workers

On 11/6/2022 2:43 AM, Roman Perepelitsa wrote:
> On Sat, Nov 5, 2022 at 10:58 PM Clinton Bunch <cdb_zsh@zentaur.org> wrote:
>>
>> helikon[~/tmp/zsh]% getrandom -L 0 -U $(( 2**32-1))
> What is getrandom?
It's a builtin in the zsh/random module I'm writing. It gets random data 
from arc4random or getrandom and falls back to /dev/urandom if those 
aren't available.
>
> Roman.
>



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-11-06 10:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-05 21:57 I think I found a serious bug in zstrtoul_underscore Clinton Bunch
2022-11-05 22:13 ` Clinton Bunch
2022-11-05 23:41   ` Clinton Bunch
2022-11-06  0:24     ` [PATCH} " Clinton Bunch
2022-11-06  5:16       ` Mikael Magnusson
2022-11-06  8:43 ` Roman Perepelitsa
2022-11-06 10:30   ` Clinton Bunch

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).