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

fix: avoid modifying slice returned by AddrsFactory #3068

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Nov 26, 2024

In some cases, AddrsFactory may return a reusable slice. This slice should not be modified.
In BasicHost.Addrs(), the slice was already being cloned before being passed to ma.Unique() (updated this place to use slices.Clone() function), but in other places, the slice was being modified without being cloned, which in some cases could lead to crashes / unpredictable behavior.

ivan4th added a commit to spacemeshos/go-spacemesh that referenced this pull request Nov 26, 2024
Also, upgrade go-libp2p-kad-dht to v0.28.1 and quic-go to v0.48.2.
There's a bug in go-libp2p related to reuse of the slices retured by
`AddrsFactory`, fix: libp2p/go-libp2p#3068
For now, the problem is mitigated by cloning the slice returned
from `AddrsFactory` (`cfg.AdvertiseAddress`).
Deprecated `max-reservations-per-peer` field in the relay server
config, as now it is always 1 (and other values didn't make too much
sense).
Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for slices.Clone ❤️

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 28, 2024

Could we merge this PR? (I have no permissions to do so...)

@sukunrt
Copy link
Member

sukunrt commented Nov 28, 2024

Yeah. I was waiting for fixing the interop test(unrelated issue). But this is fine. We can do that later.

@sukunrt sukunrt merged commit 8423de3 into libp2p:master Nov 28, 2024
8 of 9 checks passed
@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 28, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants