MdeModulePkg: Fix regressions from 11686, 11687, 11688, 11689. #11823
Conversation
|
Please could you format the PR numbers in the commit message body with a preceding hash, as you did in #11803, as this generates a link when reviewing in GitHub, which is much more convenient. |
|
Thank you for comprehensively reviewing your previous PRs. I have added some more comments on the original commits. There seem to be a few further issues that should be addressed. https://github.com/tianocore/edk2/pull/11686/files#r2579150422 https://github.com/tianocore/edk2/pull/11688/files#r2579133845 https://github.com/tianocore/edk2/pull/11689/files#r2579145214 |
| // | ||
| FieldBuffer = AsciiStrGetNewTokenField (LineBuffer, Char); | ||
| if (FieldBuffer != NULL) { | ||
| if (FieldBuffer == NULL) { |
There was a problem hiding this comment.
Why not ASSERT here? If intentional, presumably the idea must be that this case is likely/possible with reasonable input, but if so, can you briefly explain why so?
There was a problem hiding this comment.
The ASSERT() was removed, and replaced with this check, which I agree is an odd thing to do.
In general, this whole batch of CodeQL inspired 'improvements' appear to have been of highly dubious quality, resulting in new bugs and functional regressions that break the boot on various platforms.
| // | ||
| NewDevicePath = DevicePathFromHandle (FoundFvHandle); | ||
| if (NewDevicePath != NULL) { | ||
| if (NewDevicePath == NULL) { |
There was a problem hiding this comment.
Why not ASSERT here? If intentional, presumably the idea must be that this case is likely/possible with reasonable input, but if so, can you briefly explain why so?
There was a problem hiding this comment.
CodeQL should never have flagged this one. afaict. AppendDevicePathNode() accepts a NULL value for its first argument, and so there was nothing wrong with this code to begin with.
If the logic is flawed, it should be called out as such, and fixed in a separate commit, explaining why calling AppendDevicePathNode() with a NULL argument makes no sense.
@apop5 please fix this
| // Insert to Option list of current Question | ||
| // | ||
| if (ParentStatement != NULL) { | ||
| if (ParentStatement == NULL) { |
There was a problem hiding this comment.
Why not ASSERT here? If intentional, presumably the idea must be that this case is likely/possible with reasonable input, but if so, can you briefly explain why so?
There was a problem hiding this comment.
This check should be dropped entirely: it is redundant, given the check on line 2365. If the tool fails to spot that, please fix the tool.
ardbiesheuvel
force-pushed
the
personal/apop5/codeqlregressions
branch
from
8099123 to
40131e4
Compare
It is hard enough as it is to keep track of all the open issues, and so logging additional issues in PRs that are already closed is unlikely to get noticed. If you feel these issues are important, please open GitHub issues for them, and assign to @apop5. (If you are not able to do the assignment, let me know and I will take care of it) |
11686, 11687, 11688, 11689 included some inverted conditionals during the refactor. While the system booted, some behavior was incorrect based on the inverted conditionals. Signed-off-by: Aaron Pop <[email protected]>
ardbiesheuvel
force-pushed
the
personal/apop5/codeqlregressions
branch
from
40131e4 to
596af9d
Compare
|
@apop5 can you please get this fixed? |
3 participants
Description
11686, 11687, 11688, 11689 included some inverted conditionals during the refactor. While the system booted, some behavior was incorrect based on the inverted conditionals.
How This Was Tested
Ran Ovmf build, verified functionality of Display Engine, etc.
Integration Instructions
No integration necessary.