stun: drop the redundant address copy after stun_atoaddr()#336
Open
garacil wants to merge 1 commit into
Open
Conversation
stun_discovery_create() points sd_pri_info.ai_addr at sd_pri_addr, and stun_atoaddr() writes the resolved server address through info->ai_addr, so sd_pri_addr already holds the address when stun_atoaddr() returns. The follow-up copy read from &sd_pri_info.ai_addr - the address of the pointer field inside the su_addrinfo_t, not the pointed-to sockaddr - which overlaps sd_pri_addr and copied addrinfo tail bytes over the just-resolved address. Remove that copy in both stun_test_lifetime() and stun_get_nattype(); the no-server copy from sh_pri_addr is a separate valid copy and is unchanged. This also removes the -Wrestrict warning the overlapping copy produced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #335.
Both
stun_test_lifetime()andstun_get_nattype(), on the branch where a server address is supplied, copied from&sd->sd_pri_info.ai_addrright afterstun_atoaddr(). That is the address of theai_addrpointer field inside thesu_addrinfo_t, not the sockaddr it points to, so the copy overwrote the just-resolved server address insd_pri_addrwith addrinfo tail bytes (and overlapped, sincesd_pri_infoprecedessd_pri_addrin the struct).The copy is also unnecessary:
stun_discovery_create()setssd->sd_pri_info.ai_addr = &sd->sd_pri_addr->su_sa(stun.c:1148) andstun_atoaddr()writes the resolved address through that pointer (stun.c:2623), sosd_pri_addralready holds the address whenstun_atoaddr()returns.This removes the redundant copy in both functions. The no-server branch's copy from
sh_pri_addr(a different object) is a genuine copy and is left unchanged. GCC 14 at-O2also stops warning at the memcpy site (-Wrestrict, overlap).No ABI or behavior change other than preserving the resolved address instead of clobbering it.