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

ato* family of functions does not allow for robust error handling #1358

Closed
adelton opened this issue Jun 18, 2021 · 7 comments
Closed

ato* family of functions does not allow for robust error handling #1358

adelton opened this issue Jun 18, 2021 · 7 comments

Comments

@adelton
Copy link

adelton commented Jun 18, 2021

There are atoi and atol functions used in the util-linux codebase. Unlike strto* functions, they do not allow for catching errors properly, distinguishing true zero from error, or values too big.

It is possible that some of the uses are in fact fine as the goal is only to see if the value is a number (specifically the first one in cfdisk.c):

https://github.com/karelzak/util-linux/blob/master/disk-utils/cfdisk.c#L1352-L1368
https://github.com/karelzak/util-linux/blob/master/disk-utils/blockdev.c#L354
https://github.com/karelzak/util-linux/blob/master/disk-utils/fsck.c#L1607
https://github.com/karelzak/util-linux/blob/master/login-utils/utmpdump.c#L78
https://github.com/karelzak/util-linux/blob/master/sys-utils/lscpu-topology.c#L193
https://github.com/karelzak/util-linux/blob/master/sys-utils/eject.c#L538
https://github.com/karelzak/util-linux/blob/master/sys-utils/ipcs.c#L123
https://github.com/karelzak/util-linux/blob/master/sys-utils/swapon.c#L731
https://github.com/karelzak/util-linux/blob/master/sys-utils/lscpu-cputype.c#L303
https://github.com/karelzak/util-linux/blob/master/sys-utils/lscpu-cputype.c#L489
https://github.com/karelzak/util-linux/blob/master/text-utils/pg.c#L323-L327
https://github.com/karelzak/util-linux/blob/master/text-utils/pg.c#L602-L605
https://github.com/karelzak/util-linux/blob/master/text-utils/hexdump-parse.c#L136
https://github.com/karelzak/util-linux/blob/master/text-utils/hexdump-parse.c#L153
https://github.com/karelzak/util-linux/blob/master/text-utils/hexdump-parse.c#L203
https://github.com/karelzak/util-linux/blob/master/text-utils/hexdump-parse.c#L272
https://github.com/karelzak/util-linux/blob/master/term-utils/agetty.c#L2427

But if #1356 gets addressed, reviewing the ato* usage might also be useful.

@pali
Copy link
Contributor

pali commented Jun 19, 2021

Just to note that same applies also for strtou* functions. strtou* functions (which returns unsigned integers) do not signal underflow in some cases.

So the only option to check for conversion errors is to use strto* signed functions.

karelzak added a commit that referenced this issue Jun 22, 2021
It's unnecessary to use atoi in this case.

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
old version:
 # blockdev --setfra 4x096 /dev/sdc

new version:
 # blockdev --setfra 4x096 /dev/sdc
 blockdev: failed to parse command argument: '4x096'

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
* add ul_strtos64() and ul_strtou64()
* add simple test

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
* remove unnecessary strtok() use
* remove atoi use()

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 22, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 25, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jun 25, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
@karelzak
Copy link
Collaborator

All the issues should be fixed now.

@pali
Copy link
Contributor

pali commented Jun 25, 2021

There is still one small issue. Parsing number with leading space pass but with trailing fails. I guess that string with leading space should me marked as invalid number too, like string with trailing space. Or is there some specific reason why leading space in number string is accepted?

$ ./test_strutils --strtou16 '10'
'10'-->10
$ ./test_strutils --strtou16 '10 '
test_strutils: strtou16 failed: '10 '
$ ./test_strutils --strtou16 ' 10'
' 10'-->10

@karelzak
Copy link
Collaborator

man strtol:

The string may begin with an arbitrary amount of white space (as determined by isspace(3)) followed by a single optional '+' or '-' sign.

@karelzak
Copy link
Collaborator

IMHO is better to keep the current behavior as it's how we parse user input for years, but not sure why strto* behaves in this way...

@pali
Copy link
Contributor

pali commented Jun 25, 2021

man strtol:

Yes! This is source of that behavior. I know about it.

I should have rather asked if it is expected behavior that modules in util-linux also accept these leading spaces when parsing numbers.

@pali
Copy link
Contributor

pali commented Jun 25, 2021

IMHO is better to keep the current behavior as it's how we parse user input for years

Ok! So this is a good argument...

karelzak added a commit that referenced this issue Jul 20, 2021
It's unnecessary to use atoi in this case.

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
old version:
 # blockdev --setfra 4x096 /dev/sdc

new version:
 # blockdev --setfra 4x096 /dev/sdc
 blockdev: failed to parse command argument: '4x096'

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
* add ul_strtos64() and ul_strtou64()
* add simple test

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
* remove unnecessary strtok() use
* remove atoi use()

Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit that referenced this issue Jul 20, 2021
Addresses: #1358
Signed-off-by: Karel Zak <kzak@redhat.com>
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

No branches or pull requests

3 participants