ShellPkg: Fix copy-pasto in NULL check to prevent segfault

[tormodvolden]

Description

The UpdateArgcArgv() function documentation says "If OldArgv or OldArgc is NULL then that value is not returned."

However, only OldArgc was checked for NULL. In case OldArgc was non-NULL, but OldArgv was NULL, it could cause a segmentation fault.

• Breaking change?
  • Breaking change - Does this PR cause a break in build or boot behavior?
  • Examples: Does it add a new library class or move a module to a different repo.
• Impacts security?
  • Security - Does this PR have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.
• Includes tests?
  • Tests - Does this PR include any explicit test code?
  • Examples: Unit tests or integration tests.

How This Was Tested

Not tested. Not sure if any of the current code calls UpdateArgcArgv() with OldArgc non-NULL and OldArgv NULL.

Integration Instructions

N/A

[github-actions]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[tormodvolden]

keep-alive

[mdkinney]

Close and re-open to get reviewers assigned

[tormodvolden]

Thanks! I tried to close and reopen some other PRs of mine, but no reviewers were added.

EDIT: I see now they were not added immediately here either, so I may just have to be patient. EDIT 2: Indeed.

[pierregondois]

The code change itself looks ok, maybe just to be a bit more concise in the commit message:

ShellPkg: Fix check on OldArgv in UpdateArgcArgv()

Check OldArgv is not NULL before dereferencing the value.

[tormodvolden]

@pierregondois Thanks for the suggestion. I rebased to current master and amended the commit message. I think it is still worth mentioning that the bug could cause a segmentation fault.

[pierregondois]

@tormodvolden Thanks for updating the commit message. I think you could still remove this part:

However, only OldArgc was checked for NULL, probably because of
copy-pasto. In case OldArgc was non-NULL, but OldArgv was null, it could
cause a segmentation fault.

or in general, be less verbose and more factual:

• copy-pasto is more slang
• I don't think that much detail is required for the patch, the change is self-explanatory
• No need to write the commit message at the past (cf. was ...)

[tormodvolden]

I rebased to latest master.