Improve robustness of XmlTreeLib #458
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
impact:non-functional
|
@microsoft-github-policy-service agree company="Raymond Yang" |
os-d
reviewed
Apr 9, 2024
| @@ -877,14 +877,15 @@ BuildNodeList ( | |||
| // | |||
| Status = RtlXmlNextToken (&State, &Next, FALSE); | |||
| if (EFI_ERROR (Status)) { | |||
| DEBUG ((EFI_D_ERROR, "Failed to get the next token, Status = 0x%x\n", Status)); | |||
| DEBUG ((EFI_D_ERROR, "Failed to get the next token, Status = %r\n", Status)); // MS_CHANGE | |||
There was a problem hiding this comment.
The MS_CHANGE tags aren't needed for the mu_plus repo (since this entire repo represents mu value adds). For future reference, MU_CHANGE is preferred for repos that do have upstream components that we track changes against.
| // Topmost node, ignore tailing characters | ||
| // | ||
| // DONE-CCC: | ||
| // See also, TODO-AAA and TODO-BBB in `xml_fasterxml.c` |
There was a problem hiding this comment.
Let's leave full comments, please don't use annotations like DONE-CCC or TODO-AAA.
| // if (pRawToken->TokenName == NTXML_RAWTOKEN_ERROR) { | ||
| // pToken->fError = TRUE; | ||
| // success = EFI_ABORTED; | ||
| // return success; |
There was a problem hiding this comment.
Instead of commenting out the code, we can remove it. If it is helpful, do leave a full comment explaining why we don't check if the TokenName == NTXML_RAWTOKEN_ERROR.
| @@ -2946,6 +2962,14 @@ RtlXmlNextToken ( | |||
|
|
|||
| cbTotalTokenLength = pRawToken->Run.cbData; | |||
|
|
|||
| // MS_CHANGE | |||
| // | |||
| // TODO-AAA: | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Improve robustness of XmlTreeLib for handling tailing invalid (?) characters.
Current XmlTreeLib will run into endless loop if there are some non-whitespace chars after root node. Take
malloc()on Windows as an example, the memory will be initialized to0xcdon debug build. If we put data on this buffer without a proper ending char (null), theCreateXmlTree()call will never return on this buffer.This is strange. A better parser lib should (a) report this as an error or (2) stop parsing at the place.
This PR will stop parsing after first Root node was parsed, discarding any characters after it. As a side effect, if we have more than one root, only the first root will be reported. This is reasonable, since XML allows only one root node.
flow, or firmware?
validation improvement, ...
in build or boot behavior?
a function in a new library class in a pre-existing module, ...
outside direct code modifications (and comments)?
on an a separate Web page, ...
How This Was Tested
Added test cases and passed unit test on host test.
Integration Instructions
None.