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

quic: panic on error in listener Accept #2259

Closed
sukunrt opened this issue Apr 21, 2023 · 4 comments · Fixed by #2276
Closed

quic: panic on error in listener Accept #2259

sukunrt opened this issue Apr 21, 2023 · 4 comments · Fixed by #2276
Assignees
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@sukunrt
Copy link
Member

sukunrt commented Apr 21, 2023

originally reported here:
ipfs/kubo#9693

The sequence of events that causes this is:

list.Accept errors here https://github.com/libp2p/go-libp2p/blob/v0.26.4/p2p/net/swarm/swarm_listen.go#L129

and error in list.Accept has closed the listener accept loop already here:
https://github.com/libp2p/go-libp2p/blob/v0.26.4/p2p/transport/quic/virtuallistener.go#L108-L120
with sendErrAndClose

The defer cleanup in swarm_listen closes virtuallistener here https://github.com/libp2p/go-libp2p/blob/master/p2p/net/swarm/swarm_listen.go#L117

which causes panic here: https://github.com/libp2p/go-libp2p/blob/v0.26.4/p2p/transport/quic/virtuallistener.go#L73
as it checks for entry from muxer map which was removed by sendErrAndClose

@sukunrt sukunrt added the kind/bug A bug in existing code (including security flaws) label Apr 21, 2023
@MarcoPolo
Copy link
Contributor

So this is an issue with closing twice? Once in the error from accept and once on shutdown?

@sukunrt
Copy link
Member Author

sukunrt commented Apr 25, 2023

yes. Accept implicitly closes the listener on error via sendErrAndClose and the shutdown cleanup func explicitly closes it. I think this should just be an error and not a panic.

I don't know why Accept is erroring though.

@marten-seemann
Copy link
Contributor

Wild guess: might be related to quic-go/quic-go#1737?

@MarcoPolo
Copy link
Contributor

Easy fix could be to make sure we only close once. Still unclear we get the original error in Accept though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants