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 for zstd CLI accepts bogus values for numeric parameters #3268

Merged
merged 3 commits into from
Sep 21, 2022

Conversation

ctkhanhly
Copy link
Contributor

@ctkhanhly ctkhanhly commented Sep 20, 2022

Related Issue to #3070

Notes:

  • Originally, I was gonna throw an error in readU32FromCharChecked but this affects at least init_cLevel

Testing:

  • Created a test memlimit.sh

@yoniko

…eters

Signed-off-by: Ly Cao <lycao@fb.com>
@ctkhanhly ctkhanhly changed the title add checks to mal-formed numeric values for memory and memlimit param… Fix for zstd CLI accepts bogus values for numeric parameters Sep 20, 2022
@@ -0,0 +1,40 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tests !
👍

programs/zstdcli.c Outdated Show resolved Hide resolved
@Cyan4973
Copy link
Contributor

Indeed, the solution cannot be done directly within readU32FromCharChecked(),
because this function is used to read unsigned integers from the command line
in both short command (typically one letter) and --long-command= scenarios.
Erroring on wrong suffixes only makes sense in the case of the --long-command= scenario
because short commands are allowed to stack up (for example -bi7S is valid).

The --long-command= scenario happens in 6 places,
and the logic is abstracted behind the macro NEXT_UINT32().

I believe your fix is correct, but it's written directly to serve --memory= and --memlimit=, and therefore limited to these cases.

If you update NEXT_UINT32() with your corrected logic, you would fix all current and future usages of numeric parameters in --long-command= scenarios.

Copy link
Contributor

@yoniko yoniko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ctkhanhly!
Tests and logic look good.
There's one change that I'd like us to do here - there are other fields that use NEXT_UINT32 to read sizes and which use the same logic for parsing suffixes.
It'd be better to change the NEXT_UINT32 macro and so apply the same validation on those fields as well.

programs/zstdcli.c Outdated Show resolved Hide resolved
@yoniko
Copy link
Contributor

yoniko commented Sep 21, 2022

@ctkhanhly - if you have some time it'd also be useful to get the same treatment for the fields that directly use readSizeTFromChar.
The solution I'd opt for in this case is to create a NEXT_TSIZE that'd be the same as NEXT_UINT32 only it'd call readSizeTFromChar instead of readU32FromChar and use it for these fields.

programs/zstdcli.c Outdated Show resolved Hide resolved
@Cyan4973
Copy link
Contributor

Cyan4973 commented Sep 21, 2022

I see a remaining test issue in mingw-long-test (CI).

It looks completely unrelated to this PR.
Maybe the compiler version changed for example, introducing a new warning which is flagged as an error in this test.

Anyway, I think you can safely ignore it, we'll deal with it separately.

Copy link
Contributor

@yoniko yoniko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@Cyan4973
Copy link
Contributor

Thanks @ctkhanhly , great bootcamp contribution !

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

Successfully merging this pull request may close these issues.

4 participants