-
Notifications
You must be signed in to change notification settings - Fork 149
kernel_cmdline: Fix parsing/equivalence of "outside" quoted args #1739
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
Conversation
Primary motivation here is that these two should be equivalent: foo="quoted value" "foo=quoted value" This also adds tests for a few more oddball cases that weren't covered before but clarify the expected kernel behavior. Closes: bootc-dev#1737 Signed-off-by: John Eckersberg <[email protected]>
| fn test_parameter_pathological() { | ||
| // valid things that certified insane people would do | ||
|
|
||
| // quotes don't get removed from keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just incorrect before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to make foo="value" and "foo=value" equivalent when parsing kernel command line arguments. The approach of first stripping quotes from the entire argument is a good step towards this. However, the subsequent processing of the value part is now incorrect, as it no longer strips the trailing quote from a quoted value. This introduces a bug, breaking parsing for arguments like foo="value". I've suggested a fix to restore the correct value parsing logic. The added tests are a great addition for covering these edge cases, and they will pass once the parsing logic is corrected.
| let dequoted_input = input.strip_prefix(b"\"").unwrap_or(input); | ||
| let dequoted_input = dequoted_input.strip_suffix(b"\"").unwrap_or(dequoted_input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there's only one quote at the start? Shouldn't we not strip the trailing one in that case? What does the Linux kernel do in this corner case?
| let p = param("foo=\"unclosed quotes"); | ||
| assert_eq!(p.value(), Some("unclosed quotes")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is an answer to my question above, but is it the correct answer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's this line - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/cmdline.c#n256. We see '=', replace it with '\0', advance one, if it's '"' we advance again past it, effectively stripping it.
Primary motivation here is that these two should be equivalent:
foo="quoted value"
"foo=quoted value"
This also adds tests for a few more oddball cases that weren't covered
before but clarify the expected kernel behavior.
Closes: #1737
Signed-off-by: John Eckersberg [email protected]