proto_tls: report real ports when the write is queued on async connect - #4226
Open
hafkensite wants to merge 1 commit into
Open
proto_tls: report real ports when the write is queued on async connect#4226hafkensite wants to merge 1 commit into
hafkensite wants to merge 1 commit into
Conversation
proto_tls_send() only updated last_outgoing_tcp_id and send_sock->last_real_ports on the send_it path. When tls_async is enabled and no connection exists yet, the first message is attached as a write chunk to the pending connection and the function returns via con_release, leaving both stale from a previous send on that listener. The tracer module reads them when building the trace, so the very first request over a new outbound TLS connection (typically the INVITE) is logged with the ports and connection id of an unrelated peer. Every later message on that connection goes through send_it and is correct. proto_tcp already does this on both of its async paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
|
Hi @hafkensite , good catch here. Nevertheless, for code clarity, I would suggest adding the settings in the async block (as the settings of the ports are strictly related to the async op), somewhere here for example, instead of having them in the "release" section - this is something more generic (as intention), not 100% related to async stuff |
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
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.
Summary
When
tls_asyncis enabled, the first SIP request sent over a newly opened outbound TLS connection is traced (tracer/proto_hep) with the wrongto_port— and the wrong correlation id — becauseproto_tls_send()does not updatelast_outgoing_tcp_id/send_sock->last_real_portson the path where the write is only queued onto a pending connect.Details
This is a bug fix, and the scenario is a narrow one: it needs
tls_asyncenabled (the default) and a message that triggers a brand-new outbound TLS connection.socket_info->last_real_portsis per-listener state that each transport module refreshes on every send, so that the tracer can report the real IP-level ports instead of the SIP-level ones (introduced in d6a663f).modules/tracer/tracer.creads it when filling in the trace:proto_tls_send()only refreshes it just before returning fromsend_it. If no connection exists yet andtls_asyncis on,tcp_async_connect()/tls_async_connect()may not complete inline; the buffer is then attached withtcp_async_add_chunk()and the function returns viacon_release, which never touches those fields.The result is that the trace for that first message carries the ports — and the
last_outgoing_tcp_idcorrelation id — left behind by whatever was last sent from the same listener, i.e. an unrelated peer. Because the connection exists from that point on, every subsequent message on it goes throughsend_itand is traced correctly. That is what makes it look odd in practice: only the initial INVITE of a call is wrong, the rest of the dialog is fine.net/proto_tcp/proto_tcp.calready sets both fields on each of its async paths (in then==0branch aftertcp_async_connect(), and in theS_CONN_CONNECTINGbranch);proto_tlswas simply never given the same treatment.proto_ws,proto_wssandproto_msrphave no async-connect path and are not affected.Solution
Set
last_outgoing_tcp_idandsend_sock->last_real_portsat thecon_releaselabel, mirroring whatsend_itandproto_tcpdo.con_releaseis also reached whentls_async_connect()fails, where the assignment is harmless: the values are the real ports of the connection just attempted, and no trace is produced for a message that was never queued.send_sockcannot beNULLon this path —tcp_con_get_profile(to, &send_sock->su, ...)earlier in the same branch would already have dereferenced it.Compatibility
No functional change outside tracing, no config or API change, nothing to migrate.
Submitted against 3.6 as the oldest supported branch;
masterhas the identical gap (same threegoto con_releasesites) and needs the same change.