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

Added check of RequiresValidationContext before calling IsValid #110647

Merged

Conversation

vvg88
Copy link
Contributor

@vvg88 vvg88 commented Dec 12, 2024

The fix is done to avoid NRE when CompareAttribute's implementation of IsValid(object? value, ValidationContext validationContext) is called with validationContext == null.

Fix #42490

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

The fix is done to avoid NRE when CompareAttribute's implementation of IsValid(object? value, ValidationContext validationContext) is called with validationContext == null.

Fix dotnet#42490
@tarekgh
Copy link
Member

tarekgh commented Dec 12, 2024

Thank you for submitting the PR. Here are a few points regarding this change:

  • According to ValidationAttribute.RequiresValidationContext), it states: Gets a value that indicates whether the attribute requires validation context. Therefore, in my opinion, the correct behavior is to throw an exception when validating without providing the context. Allowing IsValid to return false when RequiresValidationContext is true masks the issue, failing to educate callers on the need to provide the context when using such a validation attribute. Additionally, it would not make it clear why the validation failed.
  • Any change here must be considered a breaking change, as we cannot predict how various users might have implemented their custom attributes.
  • Any proposed change should also include appropriate test cases.

CC @eiriktsarpalis @jeffhandley

@vvg88
Copy link
Contributor Author

vvg88 commented Dec 16, 2024

@vvg88 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@vvg88 vvg88 marked this pull request as ready for review December 16, 2024 06:52
@vvg88
Copy link
Contributor Author

vvg88 commented Dec 17, 2024

@tarekgh Your comments have been addressed, could you please take a look?

@vvg88
Copy link
Contributor Author

vvg88 commented Dec 23, 2024

@adamsitnik Hello! Could you please help to find a reviewer of this PR?
Thanks!
CC @jeffhandley

@tarekgh tarekgh added this to the 10.0.0 milestone Dec 24, 2024
@tarekgh
Copy link
Member

tarekgh commented Dec 24, 2024

@vvg88 thanks for helping with this change. Sorry for the delay reviewing it. You know this is holiday time and most people are off.

@tarekgh tarekgh merged commit bdf1143 into dotnet:main Dec 24, 2024
81 of 83 checks passed
@vvg88 vvg88 deleted the user/vvg88/issue-42490-compareattr-isvalid-nre branch December 24, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.ComponentModel.DataAnnotations 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.

CompareAttribute.IsValid results in null ref
2 participants