TELCORE-176: Sync FreeSWITCH fork with upstream v1.11.1 - #601
Conversation
[mod_skypopen] Remove from tree.
[mod_gsmopen] Remove from tree.
[mod_sangoma_codec] Remove from tree
[mod_ladspa] Remove from tree
[mod_cepstral] Remove from tree
[mod_sms_flowroute] Remove from tree
[mod_dahdi_codec] Remove from tree
[mod_portaudio, mod_portaudio_stream] Remove from tree
[mod_mp4, mod_mp4v, mod_mp4v2] Remove from tree
[mod_isac] Remove from tree
[mod_abstraction] Remove from tree
[mod_stress] Remove from tree.
[mod_unicall] Remove from tree
[mod_soundtouch] Remove from tree
[mod_sonar] Remove from tree
4668f86 to
50f1d16
Compare
There was a problem hiding this comment.
Rebase needed: this branch is missing TELCORE-156/TELCORE-160 trickle ICE fixes from base. Preserve them to avoid reconnect/trickle ICE regressions.
There was a problem hiding this comment.
Both are on the branch now — cherry-picked from deploy-development rather than rebased:
- TELCORE-156 (TELCORE-156: clear stale EOC/PAC state in switch_core_media_clear_ice() #613)
70e79acaad— clears stale EOC/PAC state inswitch_core_media_clear_ice(). - TELCORE-160 (TELCORE-160: fix b2bua-rtc trickle ICE NULL ufrag/pwd race #598)
5939788afc— trickle-ICE NULL ufrag/pwd race fix (touches bothswitch_core_media.candswitch_rtp.c).
I cherry-picked the 14 commits added to deploy-development since this sync branch forked instead of merging, because deploy-development sits on the 1.10.12 base and a merge would drag the version/base backward and revert v1.11.1 sync work. No trickle-ICE fixes are dropped.
There was a problem hiding this comment.
Rebase needed: this branch is missing TELCORE-160's ICE credential guard. Preserve it to avoid NULL ufrag/pwd STUN regression.
There was a problem hiding this comment.
Present now via TELCORE-160 (#598) 5939788afc. The credential guard is in switch_rtp.c: ICE activation is refused when any of the local/remote ufrag or pwd is empty —
(zstr(login) || zstr(rlogin) || zstr(password) || zstr(rpassword))
-> "Refusing to activate %s ICE: missing credentials"
so the NULL/empty ufrag/pwd STUN path is covered. Brought in by cherry-pick, not a rebase, for the base-version reason in the switch_core_media.c thread.
| dtls_fingerprint_t fp = {0}; | ||
|
|
||
| fp.type = dtls->remote_fp->type; | ||
|
|
There was a problem hiding this comment.
Rebase needed + WebRTC test coverage: this now strictly compares SDP fingerprint vs peer cert and may reject peers previously accepted.
There was a problem hiding this comment.
Same upstream change dev-ryanc flagged above — full analysis in that thread. Short version:
The pre-1.11 path was effectively a no-op. At verify time switch_core_cert_extract_fingerprint(cert, dtls->remote_fp) overwrote both remote_fp->str and remote_fp->data with the peer cert's own fingerprint, then switch_core_cert_verify() parsed ->str back into bytes and compared against ->data — i.e. it compared the cert against itself, so peer-fingerprint verification always returned true.
The new code (upstream 8babcee3ea, Fix DTLS Peer Certificate verification) compares the extracted cert fingerprint against the SDP-advertised dtls->remote_fp->str, which is the actual check.
On "may reject peers previously accepted": the formats match, so a peer presenting the cert it advertised in SDP still passes. SDP stores the fingerprint verbatim as uppercase colon-hex (switch_core_media.c, switch_set_string, no transform), extract_fingerprint emits the same %.2X: uppercase-colon form, and the compare is strncasecmp (case-insensitive). It tightens a previously-permissive check rather than breaking conformant peers.
Agreed a real-browser WebRTC DTLS smoke test is cheap insurance — it's on the verification TODO in the PR description.
| switch_stun_ip_t *ip = (switch_stun_ip_t *) attr->value; | ||
| switch_stun_ip_t *ip; | ||
|
|
||
| if (bytes_left < sizeof(switch_stun_ip_t)) { |
There was a problem hiding this comment.
Rebase needed: PRIORITY is a 4-byte attr, not an address attr. This guard can reject valid ICE/STUN packets.
There was a problem hiding this comment.
Good eye, but this is safe and it's upstream, not a rebase gap:
- The
PRIORITY-in-address-group is pre-existing. deploy-development already lumpsSWITCH_STUN_ATTR_PRIORITYin with the address attrs and does the sameswitch_stun_ip_tcast. What v1.11.1 added is the bounds check — before, that cast ran with no guard at all and would over-read on a short/truncated attr. bytes_leftis whole-packet-remaining, not attr length. At this point the 4-byte attr header has already been subtracted, so the guard rejects only when fewer than 8 bytes remain in the entire message fromattr->valueonward (andalen > bytes_leftwas already validated just above).- You're right that PRIORITY is a 4-byte value, so an 8-byte (
sizeof(switch_stun_ip_t)) floor is wider than PRIORITY strictly needs. But for that to reject a packet, PRIORITY would have to be the trailing attribute with <8 bytes after it. In real ICE connectivity checks PRIORITY is never last — USERNAME / ICE-CONTROLLING|CONTROLLED, then MESSAGE-INTEGRITY (24B) and FINGERPRINT (8B) always follow — sobytes_leftat PRIORITY is always ≫ 8 and no valid ICE packet is rejected.
Net vs deploy-development: strictly safer (unchecked over-read → bounded parse), and identical to upstream FS 1.11. If we want it pedantically correct we could move PRIORITY into the 4-byte uint32_t case group below, but since FS doesn't consume the PRIORITY value here that's cosmetic — happy to do it as a follow-up if you'd prefer.
…sion test (#585) * TELCORE-100: add ZMQ_SNDTIMEO/RCVTIMEO to Telnyx::ZMQ::ZMQSocket The wrapper only set ZMQ_LINGER, leaving SNDTIMEO/RCVTIMEO at the libzmq default of -1 (block forever). When a recovery peer became unresponsive, the call_replicator worker thread parked indefinitely inside zmq::socket_base_t::process_commands; producers kept enqueuing CDR-payload Events until the in-memory queue reached 158 GB on a prod canary host. Apply a 5 s timeout in zeromq_create_socket() so send/recv return EAGAIN instead of blocking; internal_send_request's existing reconnect logic then fires correctly. Adds tests/unit_cpp/test_zmqsocket_timeouts: three assertions (PUSH has finite SNDTIMEO, PULL has finite RCVTIMEO, sendRequest returns false on a stuck peer within a watchdog window). * TELCORE-100: drop ZMQ_RCVTIMEO from Telnyx::ZMQ::ZMQSocket Per review on PR: the receive helpers (zeromq_receive / internal_receive_*) do not check socket.recv()'s return value and treat a timed-out empty frame as a successful receive. With ZMQ_RCVTIMEO=5000 set globally on every socket, a REP socket like zmq_request_handler would synthesize a successful empty request every 5 s and call send_reply with no matching inbound REQ, violating libzmq's REQ/REP state machine (EFSM). Drop the RCVTIMEO addition for now and keep ZMQ_SNDTIMEO, which is what actually fixes the call_replicator queue leak. Auditing and fixing the receive helpers to detect timeout/EAGAIN can ship as its own change. Test loses the PULL/RCVTIMEO assertion; SNDTIMEO + behavioural test still pass (8/0).
…ry (#587) * TELCORE-125: Fix NULL deref in switch_rtp_set_flag during call recovery. When switch_rtp_new() fails (typically bind EADDRINUSE on a recovered port), switch_core_media_activate_rtp() called switch_rtp_set_flag() unguarded for the telnyx_voicemail and ignore_rtp_during_dtmf paths, dereferencing NULL at switch_rtp.c:6373. Defensive NULL guards added to switch_rtp_set_flag and switch_rtp_clear_flag, plus switch_rtp_ready() checks at the call sites to match the surrounding pattern. Regression test in tests/unit/test_rtp_set_flag_null_crash.c isolates each scenario in a forked child. * TELCORE-125: make bind-conflict test use an OS-assigned port Address PR review: the regression test hardcoded BUSY_PORT 47823 and is run under `make check` (TESTS = noinst_PROGRAMS), so it would flake on any host where that UDP port is already occupied. Bind the hog socket to port 0 and read the assigned port back with getsockname() before handing it to switch_rtp_new(), making the bind-conflict scenario deterministic. * TELCORE-125: make bind-conflict test precondition contractual Address PR review (round 2): the bind-conflict scenario was observational — it logged switch_rtp_new()'s result but did 'Do NOT assert here', so the child could exit 0 even if switch_rtp_new() returned non-NULL, in which case it never touched the recovery NULL path it claims to cover. - Enforce the precondition: the child now exits non-zero (3) if switch_rtp_new() returns non-NULL, and (4) if it fails for a reason other than the bind conflict (err must contain 'Bind Error'). Only then is the guarded switch_rtp_set_flag() call made. - Decode the child exit code in the parent (ASSERT_CHILD_OK) so a CI failure reports why (crash vs. unmet precondition vs. harness setup), not just 'exited non-zero'. - Add a direct switch_rtp_clear_flag(NULL) scenario: the production fix guards both set and clear, so cover both. Verified locally: 3/3 pass; bind-conflict child logs 'switch_rtp_new -> (nil), err=Bind Error! 127.0.0.1:44698', confirming the NULL/bind precondition is reproduced before the guarded call. * TELCORE-125: harden child-reap in NULL-crash test against EINTR/waitpid failure run_in_child() now retries waitpid() on EINTR and reports any other waitpid()/fork() failure as a distinct harness error via its return value, rather than letting a zero-initialized status be misread as a clean child exit. ASSERT_CHILD_OK() treats that harness failure as a hard fail, closing the false-pass window in a crash-regression test. * TELCORE-125: avoid uint16_t tx_port wraparound in bind-conflict test The bind-conflict regression scenario passed busy_port + 2 as the remote tx_port. tx_port is a switch_port_t (uint16_t), so when the OS assigns busy_port == 65534, busy_port + 2 narrows to 0 at the call boundary. switch_rtp_new() then rejects with "Missing remote port" before reaching switch_rtp_set_local_address(), so the NULL is not caused by the bind conflict and the child exits 4 -- a low-probability but real flake. Compute remote_port without wrap (busy_port >= 65534 ? -2 : +2). The remote port is irrelevant to the local bind conflict; it only needs to stay nonzero.
…596) * TELCORE-144: fix is_resource_available throttle logic in mod_xml_rpc is_resource_available() had three defects that combined to break all HTTP API calls carrying a query string on hosts where throttle-on-idle-cpu was configured. Manifested as HTTP 500 from the Abyss base layer (handler_hook short-circuited to its `end:` label without overriding the default 500 status set early in the function as an Abyss-assert guard). Defects: 1. Inverted ternary in is_resource_available — the default branch returned SWITCH_FALSE, so any cmd not matching `cmd == "bgapi"` was reported as unavailable once a throttle list was configured. 2. is_throttled_api used a substring search against tokens wrapped in leading+trailing spaces (" originate "), which never matched a real api_str like "originate sofia/...". The throttle never actually throttled anything via the bgapi indirection. 3. set_throttled_api_calls never cleared the static set, so reload of xml_rpc.conf with a changed throttle-api silently appended entries instead of replacing them. assert(!zstr(api)) also compiled out in release builds, becoming UB on empty input. Fix: - set_throttled_api_calls clears before parsing, accepts NULL/empty as "no throttle", tokenizes on whitespace (spaces and tabs), inserts bare token strings. - is_resource_available short-circuits to SWITCH_TRUE when no throttle is configured (empty list or MIN_IDLE_CPU<=0). Otherwise, the cmd is checked against the set directly; if it's "bgapi" and the cmd itself isn't listed, the first whitespace-delimited token of api_str is checked. If neither matches, allow. If matched, deny only when idle is strictly below the watermark. Behaviour delta (intentional): - Direct calls to listed names (e.g. /api/originate?...) are now throttled when CPU is low. Previously bypassed the throttle code path entirely. - /api/<unlisted>?<query> now returns 200 in all cases. Previously 500. - Reloading the conf with a different throttle-api now replaces the list. Previously the union accumulated across reloads. Unit test added (test/test_rpc_helper.cpp) covering 24 cases, including explicit regression checks for each of the three defects. Builds and runs without libfreeswitch via stubbed switch_core_idle_cpu/zstr/ SWITCH_TRUE etc. Wired into the module's Makefile.am via noinst_PROGRAMS + TESTS so `make check` runs it. autoreconf required after this patch (adds a TESTS target). * TELCORE-144: reset throttle helper state at start of do_config() Review follow-up. The rpc_helper.cpp setters clear correctly, but do_config() only calls them from inside `if (!zstr(var) && !zstr(val))`. So an operator who empties throttle-api ("") or removes the throttle-api / throttle-on-idle-cpu params entirely would never trigger the setters on reload, leaving the previous throttle set and watermark active — production APIs stay throttled after an attempt to disable throttling. Reset throttle helper state (set_throttled_api_calls(NULL), set_min_idle_cpu_watermark("0")) before the param-parse loop, so present config values repopulate from a clean slate and absent/empty ones leave throttling disabled. Adds two config-boundary regression tests modelling the do_config() reset sequence: throttle configured -> reload with params removed, and -> reload with throttle-api emptied. test_rpc_helper now 29/29.
Accept a no-SDP INVITE, send our own SDP offer in a reliable 183 Session
Progress (Require: 100rel), consume the answer from the PRACK, and complete
with a 200 OK / ACK carrying no SDP.
New profile param enable-3pcc-early-offer:
true - emit offer-in-183 per call; override per session with the
enable_3pcc_early_offer channel variable
dialplan - defer every no-SDP INVITE to the dialplan so it can opt in per
call via enable_3pcc_early_offer; calls that do not opt in fall
back to offer-in-200 / answer-in-ACK
Requires enable-100rel (silently disabled otherwise). The offer is emitted on
the first 183 (pre_answer / instant_ringback / relayed early media); flows that
never send a 183 fall back to offer-in-200 / answer-in-ACK. Media is terminated
locally, so bypass/proxy-media calls fall through to existing handling.
TELCORE-158
The fallback was added for MS Teams DR but Teams uses an app-layer media timer, not RFC 8445 ICE completion, so the fallback never helped Teams. Teams DR is now correctly handled by rtp_ice_role=controlling (b2bua PR #361), which sets role before media starts so this fallback never engages on those legs. PROD audit 2026-06-03 -> 2026-06-17 across US + EU b2bua: the fallback fires ~100-200x/day, and in every traced sample the peer answers within ~60-100 ms with STUN role-conflict 487, reverting us back to CONTROLLED. No fires on b2bua-rtc / b2bua-rtc-canary. The fallback never changes the call's effective role -- it's operational noise. Reverts: 4cd1201 (#602), 2c82dd1 (#600). switch_rtp.c and switch_rtp_pvt.h return to the parent of #600 (1d6120f); TELCORE-125 (#587) null-deref fix in switch_rtp_set_flag remains in place. Linear: https://linear.app/telnyx/issue/TELCORE-190
The trickle-recheck path in check_ice() activated ICE without calling gen_ice() first. When trickle candidates drained before the SDP-answer path populated local ufrag/pwd, FS armed ICE with NULL creds and produced outbound STUN USERNAME="<remote>:(null)" signed with an empty key. Peers 401'd every check -> one-way ICE -> media never established. - src/switch_core_media.c: call gen_ice() before the trickle-recheck switch_rtp_activate_ice() when local creds are missing. - src/switch_rtp.c: defensive guard in switch_rtp_activate_ice() that refuses VANILLA activation with empty login/rlogin/password/rpassword and logs at ERROR so future regressions surface loudly. Linear: https://linear.app/telnyx/issue/TELCORE-160
Remove the explicit gpt-5.2-codex model pin from reviewpr.yml so reviewpr-internal uses its default model. Linear: https://linear.app/telnyx/issue/TELCORE-234/update-reviewpryml-to-fix-fail-gha-issue
…oreign codec teardown) (#604)
…gnature The cherry-picked TELCORE-218 change used the pre-1.11 switch_regex_perform signature (int *ovector, uint32_t olen). v1.11.1 migrated to PCRE2, where the call takes (switch_regex_t **, switch_regex_match_t **). Update the call and free the match data accordingly so mod_httapi builds against the synced tree.
deploy-development's 14 new commits were already cherry-picked onto this branch (TELCORE-100/125/144/155/156/160/170/190/192/193/218/234 + 3pcc), adapted to the v1.11.1 tree where signatures changed (e.g. TELCORE-218 switch_regex_perform -> PCRE2). Verified an -X ours merge introduces no deploy-development content beyond what this branch already contains, so this merge keeps the v1.11.1 tree verbatim and only records deploy-development as a parent to clear the PR #601 merge conflict.
…ent' into damir/telcore-176-sync-freeswitch-fork-with-upstream-v1111 # Conflicts: # tests/unit/.gitignore
The merge of deploy-development brought in new unit tests whose binaries were never added to .gitignore, so every build left them showing up as untracked. Add all 15 currently-unignored programs from noinst_PROGRAMS.
|
resolved merge conflicts |
Summary
Merges upstream FreeSWITCH tag
v1.11.1(c2c59645f6) into the Telnyx fork. This carries the full v1.11.0 sync plus v1.11.1.Review follow-up (post-merge)
Review surfaced one real merge defect and several items that turned out to be non-issues after investigation. Summary below; per-comment detail is in the review threads.
Fixed — duplicate SIP Reason handling in
sofia_handle_sip_r_invite()(45bc70443a)The merge stacked upstream's new Reason-capture / 603+ detection block on top of the pre-existing Telnyx block without reconciling them, which caused:
sip->sip_reasonrendered twice perstatus >= 400response;sip_header_as_string()allocation leaked (only the second wassu_free'd) — bounded to the NUA handle home, but still wrong (pristine upstream v1.11.1 has the same non-free; the Telnyx fork was the tidy side);sip_reasonpropagated unconditionally for every failure, silently bypassing the TelnyxPFLAG_PASS_603_NETWORK_BLOCKEDgate, which became dead code.Reconciled into a single block: render once / free once, current-leg
sip_reasonunchanged, upstream'ssip_603plus_reason(ATIS-1000099) detection kept on both legs, q850 /ignore_reason_header_by_sip_codemapping kept.Partner-propagation policy — Option A (preserved): partner-leg
sip_reasonis propagated only for603 "Network Blocked"underPFLAG_PASS_603_NETWORK_BLOCKED, exactly as before the merge. This sync PR intentionally does not adopt upstream's broader "propagate partnersip_reasonfor all failures" behavior. If we later want that broadening, it should be a separate, explicitly-reviewed change.Investigated — no change needed
8babcee3ea,switch_rtp.c:4001/4267): this is a security fix, not a regression. The old path hadswitch_core_cert_extract_fingerprint()overwrite bothremote_fp->strand->datawith the peer cert's own fingerprint beforeswitch_core_cert_verify(), so it compared the cert against itself and always passed. The new code compares the cert fingerprint against the SDP-advertised one. SDP stores the fingerprint verbatim as uppercase colon-hex (switch_core_media.c:5404),extract_fingerprintemits the same form, and the compare isstrncasecmp— formats match. Recommend a quick real-browser WebRTC DTLS smoke test before deploy (cheap; non-blocking).switch_core_media.h"dropped"switch_rtp_set_ice_role()declaration: intentional. TELCORE-138 (TELCORE-138: fix ICE role for non-DTLS inbound calls with ICE #591) refactored that function out ofswitch_rtp.cinto inlinertp_ice_roledialplan-override logic (switch_core_media.c:5030-5048); declaration, definition, and caller were removed together. Not a silent drop.polycom→polyresolution (mod_sofia.c:2563-2573): "keep both" is correct — the TELCORE-67 sanitizedhold_displayand the broadened UA match are orthogonal and compose cleanly.Verification
mod_sofiabuilds clean after the Reason reconciliation (incl.test/test_603plus).make allon the standard build image; real-browser DTLS/WebRTC smoke test (see DTLS note above).Coordination
1585ca7aaf [core] Fix - add missing ice_mutex to protect dtls, which touches the same ICE/DTLS mutex region TELCORE-170: Add aggressive-nomination fallback for ICE-controlled peers #600 modifies. Whichever lands second needs a small conflict resolution — coordinating with @baloeng.