Add basic error checking to parsing nr from rw=randrw:<nr>, etc #1623
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @vincentkfu , I was wondering if you might take a quick look at this and let me know what you think. I've been burnt by this before so I think moving away from atoi is good. I can change it to allow 0 even though it doesn't make sense if we are worried there could be clients intentionally passing 0. |
axboe
reviewed
Sep 1, 2023
options.c
Outdated
| @@ -597,7 +597,22 @@ static int str_rw_cb(void *data, const char *str) | |||
| return 0; | |||
|
|
|||
| if (td_random(td)) | |||
| o->ddir_seq_nr = atoi(nr); | |||
| { | |||
There was a problem hiding this comment.
Braces go on the same line as the if. You have an odd mix here.
axboe
reviewed
Sep 1, 2023
options.c
Outdated
| free(nr); | ||
| return 1; | ||
| } | ||
| if ((val <= 0) || (val>UINT_MAX)) |
axboe
reviewed
Sep 1, 2023
options.c
Outdated
| return 1; | ||
| } | ||
| o->ddir_seq_nr = (unsigned int) val; | ||
| } |
There was a problem hiding this comment.
else and closing brace go on the same line.
|
Some style fixups needed, but I agree with moving away from atoi(). |
Previously this was parsed by just doing atoi(). This returns 0 or has undefined behavior in error cases. Silently getting a 0 for nr is not great. In fact, 0 (or less) should likely not be allowed for nr; while the code handles it, the effective result is that the randomness is gone - all I/O becomes sequential. It makes sense to prohibit 0 as an nr value in the random case. We leverage str_to_decimal to do our parsing instead of atoi. It isn't perfect, but it is a lot more resilient than atoi, and used in other similar places. We can then return an error when parsing fails, and also return an error when the parsed numeric value is outside of the ranges that can be stored in the unsigned int used for nr, along with when nr is 0. Fixes axboe#1622 Signed-off-by: Nick Neumann nick@pcpartpicker.com
aggieNick02
force-pushed
the
pcpp_parse_nr_fix
branch
from
21fdef3
to
2e0a525
Compare
|
Sorry about the brace/space issues. I think I have those corrected now. Thanks! I updated the PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously this was parsed by just doing atoi(). This returns 0 or has undefined behavior in error cases.
Silently getting a 0 for nr is not great. In fact, 0 (or less) should likely not be allowed for nr; while the code handles it, the effective result is that the randomness is gone - all I/O becomes sequential. It makes sense to prohibit 0 as an nr value in the random case.
We leverage str_to_decimal to do our parsing instead of atoi. It isn't perfect, but it is a lot more resilient than atoi, and used in other similar places. We can then return an error when parsing fails, and also return an error when the parsed numeric value is outside of the ranges that can be stored in the unsigned int used for nr, along with when nr is 0.
Fixes #1622
Signed-off-by: Nick Neumann nick@pcpartpicker.com