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

Prevent stack overflow in Attribute.cs #111225

Merged
merged 2 commits into from
Jan 16, 2025
Merged

Prevent stack overflow in Attribute.cs #111225

merged 2 commits into from
Jan 16, 2025

Conversation

leotsarev
Copy link
Contributor

fixes #35784

@teo-tsirpanis
Copy link
Contributor

The comment is incorrect, but I'm not sure whether behavior needs to change. An Attribute containing an Attribute does not necessarily lead to stack overflows; it does only if there is a cycle, and if there is, a custom override of the Equals method looks better.

Are there any real-world examples that would need this fix?

@leotsarev
Copy link
Contributor Author

Real life example was from my code (see original bug).

@leotsarev
Copy link
Contributor Author

I agree that custom Equals override should be better, but I think that framework should show correct error.

@leotsarev
Copy link
Contributor Author

I finally able to formulate why I think that need to be changed.
Debug.Assert should prevent us from errors in private code. If public endpoint could lead to some failure, Debug.Assert is not enough and we need to throw exception here.

@jkotas
Copy link
Member

jkotas commented Jan 15, 2025

There are many ways the default implementation of Equals for ValueTypes or Attributes can lead to an infinite recursion. It is not practical to prevent it.

We should delete the Debug.Assert(thisValue is not Attribute); from this code since it is an invalid assertion.

@leotsarev
Copy link
Contributor Author

@jkotas as you wish

@jkotas jkotas merged commit 0910e46 into dotnet:main Jan 16, 2025
136 of 139 checks passed
@leotsarev leotsarev deleted the patch-1 branch January 20, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime 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.

Comment in Attribute.cs is not correct
4 participants