Skip to content
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

fix for possible Null Reference Exception #112412

Merged
merged 3 commits into from
Feb 12, 2025

Conversation

dmitr1ibo4kar3v
Copy link
Contributor

Method "LoadNode" returns the nullable type "XmlNode?". So, maybe we need to check "n" for "null" here?

Found by Linux Verification Center (linuxtesting.org) with SVACE.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 11, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Comment on lines 85 to 88
XmlNode n = LoadNode(true)!;

// Move to the next node
if (n.NodeType != XmlNodeType.Attribute)
if (n != null && n.NodeType != XmlNodeType.Attribute)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null-forgiving operator in the previous line explicitly expressed that null shouldn't be returned in this case.

Reading through the implementation, null can only be returned in case of malformed xml (end element without starting element). In this case, the behavior should be reconsidered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be more appropriate to fix it like this then?

XmlNode? n = LoadNode(true);
Debug.Assert(n != null);

Copy link
Contributor Author

@dmitr1ibo4kar3v dmitr1ibo4kar3v Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip. I used option with "Debug.Assert".

@jkotas
Copy link
Member

jkotas commented Feb 12, 2025

/ba-g unrelated timeouts due to overloaded queues

@jkotas jkotas merged commit 14c4743 into dotnet:main Feb 12, 2025
78 of 85 checks passed
@dmitr1ibo4kar3v
Copy link
Contributor Author

Should i push this changes to other branches (release/*)?

@huoyaoyuan
Copy link
Member

Should i push this changes to other branches (release/*)?

No. Backporting to release branches should be done by area owner. This PR doesn't have real impact and doesn't meet the criteria.

@dmitr1ibo4kar3v dmitr1ibo4kar3v deleted the f-fix-NRE-XmlLoader branch February 12, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants