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
Milestone

Comments

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Aug 5, 2020

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
Contributor

@seankhliao seankhliao commented Aug 5, 2020

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

@martisch
Copy link
Contributor

@martisch 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 label Aug 5, 2020
@ALTree ALTree added this to the Unplanned milestone Aug 5, 2020
@smasher164
Copy link
Member

@smasher164 smasher164 commented Aug 5, 2020

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

@rolandshoemaker rolandshoemaker commented Aug 5, 2020

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 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 Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.