Skip to content

Conversation

@lidel
Copy link
Member

@lidel lidel commented Dec 20, 2025

Defensive fix to prevent panic if Addrs slice contains nil entries due to data corruption from concurrent access.

Defensive fix to prevent panic if Addrs slice contains nil entries
due to data corruption from concurrent access.

Related: ipfs/kubo#11116
@lidel lidel marked this pull request as ready for review December 20, 2025 03:41
@MarcoPolo
Copy link
Collaborator

@lidel would it be more useful to return an error here to help identify the misbehaving code paths? The proposed change is silently hiding the issue.

@MarcoPolo MarcoPolo closed this Jan 5, 2026
@MarcoPolo MarcoPolo reopened this Jan 5, 2026
address review feedback: instead of silently skipping nil addresses,
log a warning to stderr for visibility into potential data races.

this preserves graceful degradation (API responses don't break) while
still surfacing the issue for debugging.
@lidel
Copy link
Member Author

lidel commented Jan 7, 2026

@MarcoPolo good idea, but we really dont want to break API by returning err here.

as a pragmatic compromise I've pushed f2979bd which adds a stderr warning when nil is encountered (like HandlePanic does, but less noisy/scary), keeping graceful degradation

hope this is acceptable

@MarcoPolo
Copy link
Collaborator

Reading the linked issue for more context. This results in a caught panic and a returned error. I think that's the correct behavior. I don't think we should work around buggy callers.

The caller can do the same thing as this proposed change by deep cloning the addresses. But if there's concurrent access to the same memory, there's a deeper bug that needs to be fixed. If this is happening in go-libp2p I'd like to know about it.

@MarcoPolo MarcoPolo closed this Jan 7, 2026
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.

3 participants