fix(autonat): close dial-back connections to prevent memory leak - #6528
fix(autonat): close dial-back connections to prevent memory leak#6528imabdulbasit wants to merge 5 commits into
Conversation
18e6d56 to
31bc937
Compare
31bc937 to
500c5a4
Compare
imabdulbasit
changed the title
|
This pull request has merge conflicts. Could you please resolve them @imabdulbasit? 🙏 |
dariusc93
left a comment
dariusc93
left a comment
There was a problem hiding this comment.
Hi. Thanks for the PR. Left a comment on the changelog, as those could possibly be done in a separate PR for formatting/cleaning up, however reviewing the code, I dont think we should be closing the connection manually (and plus spec doesnt really state anything about keeping it open or closing the connection). We could let the connection timeout after it idle not being used for the configured amount of time instead.
|
|
||
| - Due to the refactor of `Transport` it's no longer required to create a separate transport for | ||
| AutoNAT where port reuse is disabled. This information is now passed by the behaviour. | ||
| AutoNAT where port reuse is disabled. This information is now passed by the behaviour. |
There was a problem hiding this comment.
Can you reverse this diff? Cleanup of the changelog, if any, can be done in a separate PR
| This more accurately reflects the reachability state for other peers and avoids accidental hole punching. | ||
| - The server can now test addresses different from the observed address (i.e., the connection to the server was made through a `p2p-circuit`). To mitigate against DDoS attacks, the client has to send more data to the server than the dial-back costs. | ||
| See [PR 5526](https://github.com/libp2p/rust-libp2p/pull/5526). | ||
| See [PR 5526](https://github.com/libp2p/rust-libp2p/pull/5526). |
| - Update to `libp2p-request-response` `v0.16.0`. | ||
|
|
||
| - Merge NetworkBehaviour's inject_\* paired methods (see PR 2445). | ||
| - Merge NetworkBehaviour's inject\_\* paired methods (see PR 2445). |
…-probe-connection # Conflicts: # protocols/autonat/CHANGELOG.md
|
@dariusc93 Thanks for reviewing the PR. I have reverted the formatting changes. I can make the change to let the connection time out as you suggested, but I think the go implementation also closes the connection explicitly https://github.com/libp2p/go-libp2p/blob/0d90292576f8f68f54e3ef05ee688581ee4df913/p2p/protocol/autonatv2/server.go#L377 |
|
This pull request has merge conflicts. Could you please resolve them @imabdulbasit? 🙏 |
Thanks. I did review what they did and also double check the specs and it does seem that it applies mainly to autonatv2 and not v1 (see https://github.com/libp2p/specs/blob/master/autonat/autonat-v2.md?plain=1#L83-L89 and https://github.com/libp2p/specs/blob/master/autonat/autonat-v2.md?plain=1#L104-L105), so I suppose we could do that on v2 instead. For v1, that doesnt apply so we probably shouldnt worry about that. So we may need to assure that we are getting a response back from the server before we close the connection. Thoughts? |
drop v1 changes, changelog changes
f306d1f to
fbdd3e5
Compare
…-probe-connection # Conflicts: # protocols/autonat/CHANGELOG.md
2 participants
Description
The v2 AutoNAT server dials clients back on a newly allocated port to probe reachability but never closes the resulting connection: the result is delivered over the client's original connection, while other behaviours keep the dial-back connection alive indefinitely. On QUIC each such connection pins a dedicated
quinn::Endpoint(its own UDP socket plus several MiB of buffers), so a probe-serving node leaks memory slowly but without bound. This PR closes the dial-back connection once the probe completesAI Assistance Disclosure
Tools used (required — write
noneif no AI was used): Claude CodeAttestation (required):
Notes & open questions
Change checklist