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

errors: As can loop endlessly #40589

Closed
rolandshoemaker opened this issue Aug 5, 2020 · 5 comments
Closed

errors: As can loop endlessly #40589

rolandshoemaker opened this issue Aug 5, 2020 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@rolandshoemaker
Copy link
Member

Preface: I don't think this behavior is necessarily dangerous, nor am I really sure it needs to be fixed, but wanted to hear others thoughts on it.

errors.As can enter an infinite loop if the error chain it is unwrapping contains a loop (here is an incredibly obvious example, the same thing would happen if loopErr self-referenced). While my example is quite obviously a "user did something silly" issue, the longer and more complex an error chain becomes the higher the likelihood something like this could happen and cause errors.As to sit in a tight loop when the caller tries to unwind it. Since errors.As isn't using recursion it won't cause a stack overflow and the only indication of an issue will be that the program appears to just halt.

I think in an ideal world errors.As would prevent this from happening. Obviously doing full on loop detection is a bit overkill for a relatively simple stdlib function, but adding a panic when the inner for loop exceeds some maximum number of iterations (i.e. 1000, likely to be significantly larger than any real world error chain) would at least let the user know there is something bad happening.

@seankhliao
Copy link
Member

I don't think the stdlib should be placing arbitrary limits on unwrapping errors

@martisch
Copy link
Contributor

martisch commented Aug 5, 2020

Test should be able to detect that there is a bad loop and time out. At which point the code causing the loop will be detected and can be fixed by the programmer.

If we put a limit on the depth it will be too low for some and to large (oom before reaching the limit) for others. An existing case similar to the problem is that fmt Stringer can get into an infinite recursion too.

As I assume a user can not control creating a loop directly through malicious input if the programmer doesnt want it. Being an attack vector for denial of service is the reason some std lib packages put depth limits on processing input. I dont think this applies here.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 5, 2020
@ALTree ALTree added this to the Unplanned milestone Aug 5, 2020
@smasher164
Copy link
Member

This is a dup of #34957, where I closed it saying

cycle detection should not be the responsibility of the errors package. On further thought, it makes sense to see how these functions are used in the wild before preemptively documenting its behavior.

@rolandshoemaker
Copy link
Member Author

Ah! Not sure how I missed that during my search. It sounds like there haven't been huge numbers of people hitting this issue since your issue, so documentation is probably not hugely necessary, but it does sound like it could perhaps save someone a headache if they hit this in a hypothetical feature... Either way not opposed to just closing this out if everyone is ambivalent.

@toothrot
Copy link
Contributor

toothrot commented Aug 5, 2020

I'll close this as a duplicate of #34957, but please feel free to continue discussion there, or let me know if I am mistaken.

@toothrot toothrot closed this as completed Aug 5, 2020
@golang golang locked and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants