fix(pmem): correct parent_uuid format specifier in args_dict#6283
fix(pmem): correct parent_uuid format specifier in args_dict#6283buildearth wants to merge 1 commit intoavocado-framework:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical formatting error in the pmem utility, specifically for the 'parent_uuid' argument. By correcting a missing format specifier, it ensures that 'ndctl' commands are constructed correctly, preventing potential failures or misinterpretations when persistent memory operations are performed. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a formatting bug in the parent_uuid argument for the write-infoblock command. While the fix is functionally correct, it introduces a critical command injection vulnerability. Commands are constructed using user input and executed in a shell without proper sanitization, posing a significant security risk. Input sanitization is strongly recommended to prevent this.
| "align": "-a %s", | ||
| "uuid": "-u %s", | ||
| "parent_uuid": "-p %", | ||
| "parent_uuid": "-p %s", |
There was a problem hiding this comment.
This change, while fixing a formatting bug for parent_uuid, introduces a command injection vulnerability. The parent_uuid value (and other user-provided inputs like namespace, output, and kwargs) is formatted into a command string and executed with shell=True without proper sanitization. If parent_uuid contains shell metacharacters (e.g., '; rm -rf /'), an attacker could execute arbitrary commands. To remediate, sanitize all user-provided values using shlex.quote() before formatting them into the command string. Alternatively, consider avoiding shell=True and passing the command and its arguments as a list to the execution function.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6283 +/- ##
==========================================
- Coverage 73.70% 73.67% -0.04%
==========================================
Files 206 206
Lines 22620 22634 +14
==========================================
+ Hits 16673 16675 +2
- Misses 5947 5959 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The failing CI test |
|
@harvey0100 Hi, I submitted this PR on March 9th. It has been two weeks since then. I haven't received any feedback yet, so I am writing this letter to sincerely ask if you could spare some time to review it when it's convenient for you. |
harvey0100
left a comment
There was a problem hiding this comment.
The fix is correct. Please shorten the commit header to under 72 characters -- the rest should go in the body. For example:
avocado/utils/pmem: fix parent_uuid format specifier
Change -p % to -p %s to fix incorrect formatting when passing
parent_uuid to ndctl commands.
Signed-off-by: lixintao lixintao@uniontech.com
Once the commit message is fixed, this LGTM.
Change -p % to -p %s to fix incorrect formatting when passing parent_uuid to ndctl commands. Signed-off-by: lixintao <lixintao@uniontech.com>
|
@harvey0100 Thank you for your guidance. I’ve updated the commit message according to the conventions. |
the
parent_uuidformat string inargs_dictwas"-p %", missing the format specifiers. This causes incorrect formatting when passingparent_uuidto ndctl commands.Change
"parent_uuid": "-p %"to"parent_uuid": "-p %s".