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.

Sign up for GitHub

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

Jump to bottom

[2.1] Fix IPNetwork.Contains for prefixes which are not at start of subnet range. #50967

Merged
merged 2 commits into from
Dec 9, 2023
Merged

[2.1] Fix IPNetwork.Contains for prefixes which are not at start of subnet range. #50967

merged 2 commits into from
Dec 9, 2023

Conversation

adityamandaleeka
Copy link
Member

@adityamandaleeka adityamandaleeka commented Sep 27, 2023 •
edited
Loading

[2.1] Fix IPNetwork.Contains for prefixes which are not at start of subnet range

Description

The code in IPNetwork.Contains was incorrectly handling certain cases because the way subnet membership comparison code was incorrect.

This is a backport of 3d00915 to 2.1.

6.0 PR: #31573

Fixes #6674 in 2.1

Customer Impact

The result of the Contains method could be incorrect.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Simple correctness change.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@adityamandaleeka adityamandaleeka added the * NO MERGE * Do not merge this PR as long as this label is present. label Sep 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware label Sep 27, 2023
@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Sep 27, 2023
@ghost ghost added this to the 2.1.x milestone Sep 27, 2023
@ghost
Copy link

ghost commented Sep 27, 2023

Hi @adityamandaleeka. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@adityamandaleeka
Copy link
Member Author

I'll add the template and update patchconfig once validation is complete.

@BrennanConroy
Copy link
Member

update patchconfig once validation is complete.

I think you need to update it for the tests to run with the changes.

@ghost
Copy link

ghost commented Oct 5, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 5, 2023
@adityamandaleeka
Copy link
Member Author

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 7, 2023
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adityamandaleeka adityamandaleeka requested a review from a team as a code owner December 8, 2023 01:31
@adityamandaleeka adityamandaleeka added Servicing-consider Shiproom approval is required for the issue and removed * NO MERGE * Do not merge this PR as long as this label is present. labels Dec 8, 2023
@ghost
Copy link

ghost commented Dec 8, 2023

Hi @adityamandaleeka. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@adityamandaleeka
Copy link
Member Author

@BrennanConroy Can I get another review please?
@wtgodbe Just waiting for tactics approval now.

BrennanConroy
BrennanConroy reviewed Dec 8, 2023
View reviewed changes
eng/PatchConfig.props
@@ -139,6 +139,7 @@ Later on, this will be checked using this condition:
Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv;
Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets;
Microsoft.AspNetCore.SignalR.Redis;
Microsoft.AspNetCore.HttpOverrides;
Copy link
Member

Choose a reason for hiding this comment

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

I assume this should be in a new section for 2.1.41?

Copy link
Member Author

Choose a reason for hiding this comment

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

@wtgodbe is going to handle that.

BrennanConroy
BrennanConroy approved these changes Dec 8, 2023
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

pending patch.config comment

@adityamandaleeka adityamandaleeka added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Dec 9, 2023
@ghost
Copy link

ghost commented Dec 9, 2023

Hi @adityamandaleeka. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@wtgodbe wtgodbe merged commit 5599953 into dotnet:release/2.1 Dec 9, 2023
4 checks passed
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Dec 9, 2023
wtgodbe pushed a commit that referenced this pull request Oct 24, 2024
@adityamandaleeka @wtgodbe
[2.1] Fix IPNetwork.Contains for prefixes which are not at start of s…
9edb469 
…ubnet range. (#50967)

* Fix IPNetwork.Contains behavior for prefixes which are not at start of subnet range.

Backport of 3d00915 to 2.1.

* Update patchconfig.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wtgodbe wtgodbe wtgodbe approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@Tratcher Tratcher Tratcher approved these changes

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue
Projects
Servicing
Status: Done
Milestone
2.1.x
Development

Successfully merging this pull request may close these issues.
4 participants
@adityamandaleeka @BrennanConroy @Tratcher @wtgodbe