Fix SRTP ROC retry after transport reinit being a no-op

[TheOneDenis]

Description

After a media reinit following an IP change, the SRTP transport may
receive packets whose sequence numbers have rolled over, so the RX
roll over counter (ROC) carried over from the previous session no
longer matches the sender's actual ROC. The retry logic introduced in
#2846 (fixing #2682) is supposed to try the alternative ROC candidate
when unprotect fails with srtp_err_status_auth_fail.

However, the retry candidate is selected by comparing against
srtp_get_stream_roc():

srtp_get_stream_roc(srtp->srtp_ctx.srtp_rx_ctx,
                    srtp->setting.rx_roc.ssrc, &roc);
new_roc = (roc == srtp->setting.rx_roc.roc?
           srtp->setting.prev_rx_roc.roc: srtp->setting.rx_roc.roc);

srtp_get_stream_roc() reports the ROC stored in libsrtp's replay
database. The ROC set via srtp_set_stream_roc() (e.g. during
create_srtp_ctx() when the carried-over ROC is applied) is kept by
libsrtp in a separate pending state and is only folded into the
replay database once a packet has been unprotected successfully.

Since the retry exists precisely for the situation where no packet
authenticates yet, the replay database still holds its initial
value, the comparison re-selects the ROC that is already pending, and
the alternative candidate (prev_rx_roc) is never tried. Every
subsequent packet fails authentication for the rest of the call →
one-way audio (RX dead).

Instead of copying libsrtp's pending ROC state, this PR tracks which of
the two ROC candidates is currently applied in the transport itself, so
the candidates alternate on each retry without depending on libsrtp
internals. The setting is only updated when a packet authenticates: the
winning ROC is written back to setting.rx_roc.roc, so a later SRTP
restart re-applies the correct one, and prev_rx_roc.ssrc is cleared
to close the retry window. The window also closes when
srtp_set_stream_roc() fails, which then logs once and stops. The
retry and the window close are bound to the SSRC the candidates belong
to, so a packet of a different SSRC authenticating first (through the
wildcard template) neither triggers nor closes the ROC retry.

Additional review fixes:

• The RX policy in create_srtp_ctx() is now a policy list: a
  ssrc_specific stream for the known SSRC plus a ssrc_any_inbound
  template (rx_.next), so the stream exists when the retry calls
  srtp_set_stream_roc() instead of failing with bad_param on the
  template, while a remote SSRC change is still accepted through the
  template after probation (previously the specific-only policy
  dropped such packets forever, as srtp_get_stream() does not search
  templates).
• The policy condition mirrors need_retry, including
  prev_rx_roc.ssrc == rx_roc.ssrc, so a leftover prev_rx_roc with
  a different SSRC cannot select the specific stream.
• pjmedia_transport_srtp_get_setting()/modify_setting() take the
  transport mutex, so their whole-setting copies are serialized with
  the media-thread writes.
• The rx_roc/prev_rx_roc docs in transport_srtp.h mention that
  the transport updates them itself.
• A failing srtp_set_stream_roc() is logged instead of silently
  skipping the retry.
• Runt packets that cannot hold an RTP header are rejected before any
  header field is read (the seq read below and the pre-existing
  ssrc read were out-of-bounds for datagrams < 12 bytes), logged at
  level 5.
• The "Failed to unprotect SRTP" log includes the RTP sequence
  number, which tells whether the sender has passed a sequence
  rollover.

Motivation and Context

Customers lose audio when the network changes mid-call. Measured on a
16-minute production call with three network changes: the two changes
at ROC 0 kept audio working, the one change at ROC 1 killed RX audio —
same call, same SSRC, same key, ROC being the only differing variable.

Related: #2682 (one-way audio after IP change, fixed by #2846) — this
fixes a bug in the retry logic introduced by #2846 itself. #2531
maintains the RX ROC across SRTP restarts and is not affected.

How Has This Been Tested?

• Full build: ./configure && make -j3, zero new warnings.

• pjmedia-test: 8/8 passed, including the new transport_srtp_test,
  which covers the ROC retry path over an in-memory loopback transport
  pair:

  • retry recovery with the winning ROC written back to the setting
    (both directions: winner 0 and winner 1) and the retry window
    closed afterwards,
  • a remote SSRC change after the probation period still accepted
    through the wildcard template,
  • a packet of a different SSRC authenticating first neither triggers
    nor closes the ROC retry,
  • a failed retry does not leak a candidate that has not
    authenticated into the setting.

  The test fails against the unpatched transport_srtp.c (no retry
  recovery), when the wildcard template is dropped from the policy
  list, and when the retry state is kept in the setting instead of
  the transport, so a libsrtp or policy change cannot silently regress
  this.

• A --disable-libsrtp build links and runs pjmedia-test (7 tests,
  the SRTP one compiled out).

• Field test on iOS (2.17 with this patch), a ~30 min call with two
  network handovers 11 seconds apart:

  • the retry fired once with the new format:
    Initializing SRTP RX ROC from SSRC 938613861 with ROC 1 succeeded
    followed by Retrying to unprotect SRTP from ROC 1 to new ROC 0
    and immediate jitter buffer recovery (11 ms later) — audio
    survived both handovers
  • the second handover needed no retry at all: the winning ROC had
    been written back to the setting, so the restart applied the
    correct one
  • no Failed to unprotect SRTP during the entire call

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the CODING STYLE of this project (C89, minimal comments).
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[CLAassistant]

All committers have signed the CLA.

[nanangizz]

Thank you for your contributions.
Please sign the CLA for this PR as well as the other one you submitted.

[nanangizz]

Thanks for the PR, the analysis is correct: srtp_get_stream_roc() returns the replay database ROC, not the pending one set by srtp_set_stream_roc(), so the retry never alternated. With rx_roc_pending the two candidates now alternate as intended, and I don't see a regression in the related paths (SSRC-change restart from #4490, replay restart, remote-address-change case).

A few minor comments:

1. The "Retrying to unprotect SRTP from ROC %d to new ROC %d" log still prints roc from srtp_get_stream_roc(), which is the misleading value. Please print the previous rx_roc_pending instead.
2. The TEST_ROC block in srtp_rtp_cb() calls srtp_set_stream_roc() without updating rx_roc_pending. It's debug-only, but it would be good to update it there as well for consistency.
3. In the description: the ROC retry logic was added by Add feature to init SRTP ROC and check if SRTP ROC needs to be reset/maintained #2846 (fixing One-Way-Audio (TX only) caused by SRTP ROC reset (media reinit) after IP change #2682), not SRTP unprotect fails after several media updates/SDP negotiations #2531. SRTP unprotect fails after several media updates/SDP negotiations #2531 only maintains the RX ROC across SRTP restarts.

[TheOneDenis]

Thanks for the review and the corrections — good points, especially the #2846 vs #2531 mix-up. We'll address all three comments and update the PR shortly.

[TheOneDenis]

Hi nanang,

Thanks for the review and the confirmation of the analysis. All three points are addressed:

1. The retry log now prints the previous rx_roc_pending (captured before the new ROC is set) instead of the srtp_get_stream_roc() value — and the srtp_get_stream_roc() call was removed from the retry block entirely, since it only ever reported the misleading replay-database value.
2. The TEST_ROC debug block now also updates rx_roc_pending after srtp_set_stream_roc() succeeds.
3. The PR description is corrected: the retry logic was introduced by Add feature to init SRTP ROC and check if SRTP ROC needs to be reset/maintained #2846 (fixing One-Way-Audio (TX only) caused by SRTP ROC reset (media reinit) after IP change #2682); SRTP unprotect fails after several media updates/SDP negotiations #2531 only maintains the RX ROC across SRTP restarts.

We also ran a field test on the reworked patch (2.17 with this PR applied): a 23m50s call — past the RTP sequence rollover, so RX ROC >= 1 — with two WLAN handovers. After the second handover, the retry fired and alternated as intended:

12:53:15.095  Initializing SRTP RX ROC from SSRC 834596058 with ROC 1 succeeded
12:53:15.115  Retrying to unprotect SRTP from ROC 1 to new ROC 0
12:53:15.115  ... Jitter buffer starts returning normal frames (after 4 empty/lost)

Audio survived both handovers; the call-CDR shows continuous media for the full call duration (stream duration = call duration, onewayrtp=0, MOS 4.32-4.40) with a regular BYE termination, and no Failed to unprotect SRTP during the entire call.

[sauwming]

nanangizz's three points look addressed in da01bfec:

1. The retry log now prints the previous pending ROC (old_roc), and the srtp_get_stream_roc() call is gone.
2. The TEST_ROC block updates rx_roc_pending after srtp_set_stream_roc() succeeds.
3. The description now credits #2846 (fixing #2682) for the retry logic.

A few issues remain, see inline. The first two can leave RX dead, so I think they need fixing before merge. The libsrtp behaviour below was checked against third_party/srtp/srtp/srtp.c, not tested at runtime.

[sauwming]

Regression: this can re-arm a stale pending_roc that libsrtp never clears.

libsrtp only clears pending_roc in the srtp_err_status_pkt_idx_adv branch of srtp_unprotect(). When the estimate falls within seq_num_median of the replay DB index, the packet authenticates and pending_roc stays set. So the comment above is wrong, and zeroing our copy here makes it drift from libsrtp's state.

Scenario (local change, rx_roc.roc=N, prev_rx_roc.roc=0, remote kept its ROC):

1. The first packet authenticates at ROC N (the index advances, so libsrtp folds and clears it). We set rx_roc_pending = 0.
2. Still within probation, one packet fails auth (corrupted, or forged by someone who saw the SSRC). old_roc = 0 != N, so new_roc = N and srtp_set_stream_roc(N) sets pending_roc = N.
3. Later valid packets are close to the DB index, so they return ok, not pkt_idx_adv. pending_roc is never cleared, and every packet is estimated with a fixed ROC N.
4. At the sender's next seq wrap, the estimate is (N<<16 | small), far below the DB index, and every packet fails with pkt_idx_old. That isn't auth_fail and probation is over, so RX stays dead (one-way audio).

The old code compared against the DB ROC (N), chose prev (0) and called set_stream_roc(0), which only clears pending_roc, so it was harmless here.

Suggested fix: stop ROC retries once any packet has authenticated. Alternatively, keep the ROC that just authenticated instead of zeroing it.

[sauwming]

The retry never runs in the remote-change case (rx_roc.roc=0, prev_rx_roc.roc=N).

pjsua_media.c (~3993-4001) sets rx_roc.roc=0 and prev_rx_roc=<old ssrc, N> on a remote change. create_srtp_ctx() then sees rx_roc.roc == 0, builds the RX policy with ssrc_any_inbound and skips srtp_set_stream_roc(). If the remote didn't reset its ROC, unprotecting with the template fails with auth_fail. We then call srtp_set_stream_roc(rx_ctx, rx_roc.ssrc, N), but srtp_get_stream() only searches stream_list, not the template, so the call returns srtp_err_status_bad_param. The if (status == srtp_err_status_ok) check skips the retry with no log, and this repeats for the whole probation window.

So the two candidates only alternate in the local-change case. At minimum, please log this failure. Ideally, make the remote-change case work too, e.g. by creating the stream so the ROC can be set on it.

[sauwming]

create_srtp_ctx() is also called for the DTLS RTCP context (transport_srtp_dtls.c:962, &ds->srtp->srtp_rtcp) when the RTCP handshake finishes without rtcp-mux. That can happen after the RTP context was already started with rx_roc.roc=N. This reset then clears the per-transport rx_roc_pending while libsrtp's RTP stream still has pending_roc=N. The next retry computes old_roc=0, re-sets the same failed ROC N, and only tries prev_rx_roc one packet later.

The reset and the assignment below should only apply when ctx == &srtp->srtp_ctx.

[sauwming]

Design concern: rx_roc_pending copies libsrtp's private pending_roc state, and the copy drifts easily (see the comment on line 1848). libsrtp treats pending_roc == 0 as "none", so srtp_set_stream_roc(..., 0) doesn't select ROC 0. It falls back to the replay DB estimate, which is only ROC 0 while the DB is fresh.

A simpler option that doesn't depend on libsrtp internals: keep a boolean "next retry uses prev" toggle, and only retry while no packet has authenticated yet (e.g. an rx_authenticated flag set on first success, or clear prev_rx_roc.ssrc then).

[sauwming]

Nit: this reads the RTP header without checking the packet length. A runt datagram (for example 1-3 bytes) gets here with bad_param and reads seq past size, which is an out-of-bounds read under ASan. The rx_ssrc read at line 1733 already has the same problem. One size >= sizeof(pjmedia_rtp_hdr) check early on would cover both.

[sauwming]

Minor: the comments this PR adds are fairly long (here, 1807-1812, 1845-1847). Per the coding style, keep comments minimal. The struct field comment is enough to explain the libsrtp detail, and the other two could be one line or dropped. The 1845 comment is also inaccurate (see the comment on line 1848).

[TheOneDenis]

Hi sauwming,

Thank you for the thorough review — and for checking the libsrtp behaviour against the source. We verified your analysis point by point against third_party/srtp/srtp/srtp.c and reworked the PR accordingly (commit e03e1f8):

1. Stale pending_roc re-arm (and 4. design concern): Agreed — copying libsrtp's pending_roc was the wrong model, and we confirmed it is only cleared in the pkt_idx_adv branch. The rx_roc_pending field is gone. Instead, the transport now tracks the retry state itself:

• rx_roc_retry opens the retry window when the RTP context is created and is closed as soon as any packet has been authenticated. Retries are gated on it (need_retry now requires srtp->rx_roc_retry), so a spurious auth failure after the stream is up can no longer re-arm a wrong ROC at all.
• rx_roc_retry_alt tells which candidate is currently applied, so the two candidates alternate on each retry without depending on libsrtp internals (and without the pending_roc == 0 special case).

2. Remote-change case (rx_roc.roc=0, template stream): Confirmed, and we can pinpoint it now: in create_srtp_ctx() the RX policy only picks ssrc_specific when setting->rx_roc.roc != 0 — the remote-change case (rx_roc.roc=0) always gets ssrc_any_inbound, so the stream only exists as a template and srtp_set_stream_roc() returns bad_param. The failure is now logged ("Failed to set RX ROC %d for retry, err=%s") instead of being silently skipped. Making the retry actually work there seems to require choosing ssrc_specific when prev_rx_roc.roc != 0 and the SSRC is known (or materializing the stream another way) — that changes the SSRC policy selection and interacts with the SSRC-change restart path (#4490), so we did not want to decide it unilaterally in this PR. Happy to implement it here if you prefer, or as a follow-up — your call.

(Re: the TEST_ROC block from the earlier round — with the boolean design there is no copied libsrtp state left to update. TEST_ROC applies exactly setting.rx_roc.roc, which is what rx_roc_retry_alt = FALSE reflects, and the retry cannot fire in a fresh session anyway because need_retry requires prev_rx_roc.ssrc != 0.)

3. DTLS RTCP context: Fixed — the retry-window initialization is now guarded by ctx == &srtp->srtp_ctx, so the RTCP context creation no longer resets it.

5. Runt packet OOB read: Fixed — an early size < sizeof(pjmedia_rtp_hdr) check drops runt datagrams before any header field is read, covering the seq read added by this PR and the pre-existing ssrc read.

6. Comments: Shortened — the struct field comment explains the retry state, the other two are one-liners or dropped.

We also re-ran the field test on the reworked patch: a 24m26s call (past the RTP sequence rollover, so RX ROC >= 1) with two WLAN handovers nine seconds apart. The retry fired once with the new format, followed by immediate recovery:

12:12:51.803  Initializing SRTP RX ROC from SSRC 227927120 with ROC 1 succeeded
12:12:51.905  Retrying to unprotect SRTP from ROC 1 to new ROC 0
12:12:51.907  Jitter buffer starts returning normal frames

No re-arm occurred on the second handover: the stream had already authenticated, the retry window was closed, and audio stayed up. No Failed to unprotect SRTP for the entire call.

[sauwming]

Thanks for the rework. Points 1, 3, 4 and 5 from the previous review are resolved: the boolean retry window is much cleaner, and I checked it against the bundled libsrtp. In the local-change case the candidates now alternate correctly. Point 2 (remote-change) is answered in a separate comment.

A few smaller things remain, see inline. There is also no automated test for the ROC retry path. Since the fix depends on libsrtp's pending_roc behaviour, a future libsrtp upgrade could quietly turn the retry back into a no-op. A pjmedia-test case with two SRTP transports and a preset rx_roc/prev_rx_roc mismatch could reproduce it deterministically without a real IP change. Nice to have, not a blocker.

[sauwming]

If srtp_set_stream_roc() fails, rx_roc_retry stays PJ_TRUE. Every auth-failed packet in probation (~100, PROBATION_CNT_INIT) then retries an operation that can't succeed, and each one logs this at level 4. Please set srtp->rx_roc_retry = PJ_FALSE; in this branch so it logs once and stops.

[sauwming]

When the alternative candidate (prev_rx_roc) is the one that authenticates, setting.rx_roc.roc still holds the other candidate. need_restart restarts via pjmedia_transport_srtp_start() → create_srtp_ctx(&srtp->setting), so a later restart in probation (e.g. a late old packet giving replay_old) re-applies the wrong ROC and resets rx_roc_retry_alt. The packet that triggered the restart is then unprotected once with the wrong ROC and dropped, and the next one needs another retry. It recovers, but loses packets.

Consider updating setting.rx_roc.roc to the ROC that authenticated (or swapping it with prev_rx_roc.roc) when a retry succeeds.

[sauwming]

This fires for every runt packet at level 4, so anyone who can reach the RTP port can flood a default-level log. Please use level 5 to match the other per-packet drop log (Failed to unprotect SRTP).

Nit: pj_ssize_t is pj_int64_t on PJ_WIN64, so %ld gives -Wformat on MinGW-w64. The existing logs have the same issue, but a (long)size cast here avoids adding another.

[sauwming]

Minor: this comment and the one at 1817-1819 mostly repeat the struct field comment. They could be dropped, or cut to one line each.

[sauwming]

@TheOneDenis thanks for the clear write-up on the remote-change case (rx_roc.roc=0, template stream).

Yes, please implement it here. Choosing ssrc_specific when the SSRC is known and prev_rx_roc.roc != 0 sounds like the right direction. Please also check that it still works with the SSRC-change restart path from #4490.

Thank you!

[TheOneDenis]

Hi sauwming,

Implemented in commit a7375288. The policy selection in create_srtp_ctx() now picks ssrc_specific when the SSRC is known and either ROC candidate is set:

if (setting->rx_roc.ssrc != 0 &&
    (setting->rx_roc.roc != 0 || setting->prev_rx_roc.roc != 0))
{
    rx_.ssrc.type       = ssrc_specific;
    rx_.ssrc.value      = setting->rx_roc.ssrc;
}

So in the remote-change case the stream exists when the retry calls srtp_set_stream_roc(): the first packet fails auth against the fresh replay database (estimated ROC 0, remote at N), need_retry fires, and the retry now successfully applies prev_rx_roc.roc — no more bad_param on the template.

Re: #4490 — traced and unaffected. If the remote instead sends a new SSRC, unprotect returns no_ctx, which is still handled by PJMEDIA_SRTP_CHECK_SSRC_ON_RESTART: need_restart resets rx_roc.ssrc, the session restarts, and with rx_roc.ssrc == 0 the policy falls back to ssrc_any_inbound again, so the new SSRC clones from the template exactly as before. The DTLS RTCP context is also unaffected: transport_srtp_dtls.c builds its setting via pjmedia_srtp_setting_default(), so rx_roc/prev_rx_roc are zeroed there and the new condition never triggers.

One honest corner-case note: if the remote changes its SSRC and keeps its ROC at the same time, the restarted session has rx_roc.ssrc = 0, and since need_retry requires prev_rx_roc.ssrc == rx_roc.ssrc, no ROC retry can run for the new SSRC. In practice a new SSRC implies a new SRTP context (ROC reset per RFC 3711 §3.3.1 after re-keying), so we left that as-is — happy to discuss if you see it differently.

Build is clean (make -j3, no new warnings), pjmedia-test 7/7.

[sauwming]

Thanks for implementing the remote-change case and for tracing #4490. The ssrc_specific choice I suggested has a side effect I missed (see inline on line 1064), and the condition is a bit wider than the retry needs (line 1065).

Four comments from my 2026-09-30 review are still open:

• Line 1827: when the alternative ROC authenticates, setting.rx_roc.roc still holds the other candidate, so a restart in probation re-applies the wrong ROC.
• Line 1834: please set rx_roc_retry = PJ_FALSE when srtp_set_stream_roc() fails. It's mostly unreachable now, but still worth having.
• Line 1672: please use level 5 for the runt-packet log and add a (long)size cast.
• Lines 1089 and 1817: please drop these comments or cut them to one line each.

Optional, but it may simplify things: on the first authenticated packet, store the winning ROC in setting.rx_roc.roc and clear setting.prev_rx_roc.ssrc. That would close the retry window for later restarts and sessions too, replace rx_roc_retry, and address both line 1827 and line 1065.

[sauwming]

With ssrc_specific in the remote-change case there's no longer an ssrc_any_inbound template in the RX context. As you traced, a new SSRC from the remote within probation still works via the no_ctx restart. But PJMEDIA_SRTP_CHECK_SSRC_ON_RESTART only runs while probation_cnt > 0, so an SSRC change after probation (e.g. an SBC switching media source with the same SDES keys) now gets no_ctx on every packet, and incoming audio dies. Before this commit, libsrtp would clone a stream from the template.

Note that the rx_roc.roc != 0 case already had this exposure before this PR; this commit extends it to the remote-change case. Could you check whether giving srtp_create() a policy list works here, i.e. the ssrc_specific stream plus an ssrc_any_inbound template via rx_.next? That would keep the stream available for srtp_set_stream_roc() and still accept new SSRCs. I haven't verified how libsrtp handles that combination.

[sauwming]

need_retry also requires prev_rx_roc.ssrc == rx_roc.ssrc, but this condition doesn't. pjsua_media.c starts from pjmedia_transport_srtp_get_setting() and only overwrites prev_rx_roc on a local/remote change, so a prev_rx_roc left over from an earlier re-INVITE can still pick ssrc_specific for a session with a different SSRC, where no retry can ever run. Please add setting->prev_rx_roc.ssrc == setting->rx_roc.ssrc (for the prev_rx_roc.roc != 0 part) so the policy matches the retry condition.

[TheOneDenis]

Hi @sauwming, thanks for the thorough review. All points addressed in e623ff049 (plus a test in 359f8079d):

1. L1064 — template loss: The RX policy is now a policy list: rx_ (ssrc_specific) + rx_tmpl (ssrc_any_inbound) linked via rx_.next. As you suspected, the template is what accepts a remote SSRC change after probation; a packet with a new SSRC is authenticated through the provisional stream and cloned into a real one (srtp_unprotect() → srtp_stream_clone()). Verified against third_party/srtp/srtp/srtp.c: one ssrc_any_inbound template and ssrc_specific streams coexist in one session, and srtp_get_stream() only searches stream_list, which is why keeping the template is essential.

2. L1065 — condition width: The policy condition now mirrors need_retry exactly, including setting->prev_rx_roc.ssrc == setting->rx_roc.ssrc, so a leftover prev_rx_roc with a different SSRC cannot select the specific stream.

3. L1827 — wrong ROC re-applied on restart: The two retry-state booleans are gone. Each retry now swaps setting.rx_roc.roc and setting.prev_rx_roc.roc, so rx_roc.roc always holds the currently applied candidate — and after a packet authenticates, the winning ROC. The window closes via prev_rx_roc.ssrc = 0 on success, and also when srtp_set_stream_roc() fails (L1834), which then logs once and stops. The ctx == &srtp->srtp_ctx special case is removed entirely.

4. L1672: runt-packet log moved to level 5 with a (long)size cast. Comments trimmed to one-liners.

5. Automated test (your "nice to have"): 359f8079d adds a pjmedia-test case over an in-memory loopback transport pair, exercising the full protect/unprotect path:

• retry recovery with the winning ROC written back to the setting (both directions: winner 0 and winner 1), retry window closed afterwards,
• a remote SSRC change after the probation period still accepted through the wildcard template.

It fails against the unpatched transport_srtp.c (no retry recovery) and when the wildcard template is dropped from the policy list — so a future libsrtp or policy change cannot silently regress this.

Also field-tested in a ~30 min call with two network handovers: exactly one Retrying to unprotect SRTP from ROC 1 to new ROC 0, the jitter buffer returned to normal 11 ms later, and the second handover needed no retry at all — the winning ROC had been written back to the setting, so the restart applied the correct one.

[sauwming]

Thanks, all the inline comments from my previous reviews are addressed. The template in the policy list looks good, and thanks for adding the test.

Two test issues need fixing before merge (test.c:119, test.h:42), and there are three small edge cases in the fix itself (inline on transport_srtp.c).

Minor, can be done together:

• transport_srtp.h: the docs for rx_roc/prev_rx_roc should mention that the transport now updates them itself: it swaps the ROC candidates and clears prev_rx_roc.ssrc after the first authenticated packet. Callers of pjmedia_transport_srtp_get_setting() will see the changed values.
• transport_srtp_test.c: fixture_init() returns early on errors without closing the SRTP transports it already created, and the callers don't call fixture_destroy() then, so they leak under ASan.
• transport_srtp_test.c: please wrap CHECK() in do { ... } while (0) (as written, a caller's else would attach to the macro's if), and add a final newline.
• pjmedia_test.vcxproj.filters: the new file is missing there (cosmetic; sdp_attr_test.c has the same gap).

[sauwming]

transport_srtp_test creates and destroys its own pjmedia_endpt, so it has the same def_codec_mgr race described in the comment just below for codec_test_vectors. If it overlaps with sdp_neg_test, that test can use the codec manager mutex after it's destroyed, which gives intermittent CI failures. Please register it with PJ_TEST_EXCLUSIVE.

[sauwming]

Please guard this. With --disable-libsrtp (PJMEDIA_HAS_SRTP=0), pjmedia-test fails to link because the pjmedia_transport_srtp_* symbols are missing. With PJMEDIA_SRTP_CHECK_ROC_ON_RESTART=0 the retry never runs, so the retry/write-back scenarios always fail. For example:

#define HAS_TRANSPORT_SRTP_TEST (PJMEDIA_HAS_SRTP && \
                                 PJMEDIA_SRTP_CHECK_ROC_ON_RESTART)

[sauwming]

Two small issues here:

1. The window can close on the wrong SSRC. With the template in the policy list, a packet from a different SSRC (same key) can authenticate first and clear prev_rx_roc.ssrc. Every later packet from rx_roc.ssrc then fails with auth_fail, and need_retry never fires again, so incoming audio stays dead in the remote-change case. Please only close the window when rx_ssrc == srtp->setting.rx_roc.ssrc.
2. Unlocked writes to srtp->setting on every packet. This runs for every authenticated packet in the media thread, while pjmedia_transport_srtp_get_setting()/modify_setting() copy the whole struct without the lock from the SIP thread (e.g. pjsua_media.c around line 3980 during a re-INVITE). Before this PR, setting was only written on rare restarts. Writing only when it changes avoids the constant race:

if (srtp->setting.prev_rx_roc.ssrc != 0 &&
    rx_ssrc == srtp->setting.rx_roc.ssrc)
{
    srtp->setting.prev_rx_roc.ssrc = 0;
}

Taking srtp->mutex in get_setting()/modify_setting() would make it fully safe, but that can be a follow-up.

[sauwming]

The swap writes the candidate being tried into setting.rx_roc.roc before it has authenticated. While rx_ssrc == 0 (it's reset in pjmedia_transport_srtp_stop()), transport_get_info() reports setting.rx_roc, and pjsua_media.c saves that into prev_srtp_info.

Scenario (local change, rx_roc={S,N}, prev_rx_roc={S,0}, sender is at N): after an odd number of failed retries, setting.rx_roc.roc is 0. If a second handover arrives before any packet authenticates, pjsua starts the new session with rx_roc={S,0} and prev_rx_roc={S,0}. need_retry requires the two ROCs to differ, so N is never tried again and incoming audio stays dead.

Suggestion: keep the original setting unchanged during retries, track the currently applied candidate in a local/transport field (as in the previous rx_roc_retry_alt), and write the ROC back to setting.rx_roc.roc only once a packet has authenticated.

[TheOneDenis]

Hi @sauwming, thanks again — all inline points are addressed in 8b36bf229, the minor list in c115beeb1, and your two transport_srtp.c findings are locked by test scenarios as well:

L1857 (1) — window closes on the wrong SSRC: The window now closes only when rx_ssrc == setting.rx_roc.ssrc, so a packet of a different SSRC authenticating first (through the template) leaves the retry intact. I also added the same guard to need_retry itself — it had the symmetric hole: a template packet failing auth could trigger a ROC retry for another SSRC's stream, applying a candidate there and logging per packet.

L1857 (2) — per-packet setting writes: Gone. The setting is now written only while the retry window is open, at most once (see below).

L1819 — swap leaks an unauthenticated candidate: Agreed, the swap was writing the candidate being tried into setting.rx_roc.roc. The transport tracks the applied candidate in a field again (rx_roc_retry_alt, reset when the RTP context is created, still guarded by ctx == &srtp->srtp_ctx for the DTLS RTCP case), and the setting is only touched when a packet authenticates:

if (srtp->setting.prev_rx_roc.ssrc != 0 &&
    rx_ssrc == srtp->setting.rx_roc.ssrc)
{
    if (srtp->rx_roc_retry_alt)
        srtp->setting.rx_roc.roc = srtp->setting.prev_rx_roc.roc;
    srtp->setting.prev_rx_roc.ssrc = 0;
    srtp->rx_roc_retry_alt = PJ_FALSE;
}

So your second-handover scenario keeps its original candidates until the stream authenticates, and the write-back still satisfies the restart concern from your earlier round (winning ROC in rx_roc.roc once audio is up).

test.c — PJ_TEST_EXCLUSIVE: Done, with a one-line comment referencing the codec_test_vectors rationale.

test.h — guard: Done as suggested; the test file compiles to a stub returning 0 when disabled. Verified with an actual --disable-libsrtp build: pjmedia-test links and runs (7 tests, the SRTP one absent).

Mutex in get_setting()/modify_setting(): Took it in this PR rather than as a follow-up — the media-thread writes already run under srtp->mutex, so those two copies are now serialized with them (transport_get_info() still reads a few setting fields without the lock, as before).

Minor list: transport_srtp.h documents that the transport updates rx_roc/prev_rx_roc itself; fixture_init() closes what it created on every error path (fixture_destroy() is NULL-safe); CHECK() is do { } while (0)-wrapped with a trailing newline; the filters file entry is added.

Test coverage for the two findings: two new scenarios — a different-SSRC packet authenticating first (retry for the known SSRC must still recover, window must stay open), and a sender at a ROC neither candidate matches (odd number of failed retries must leave the setting untouched and the window open). Both fail against the previous swap design, so the exact bugs you found cannot reappear unnoticed. pjmedia-test is 8/8 with SRTP, 7/7 without.

[TheOneDenis]

Hi @sauwming, the Default / pjsua-test (CI MacOS) job failed on 200_register.py:

2/288: scripts-run/200_register.py... failed!! (retried 3x)
====== Caught error: client : Timeout expecting pattern:
"<sip:test1@pjsip.org>.*registration success" ======

This looks like an environmental flake rather than anything from this PR: 200_register.py registers against the external sip:sip.pjsip.org registrar, no SRTP or media involved. The same test failed identically on master earlier today (run 37405439370, 02:52 UTC, two jobs there flaked), and in this very run the ioq-cb-with-lock / pjlib,pjsua job ran the same 288-test suite on the same commit and passed, including 200_register.py. All SRTP tests passed in the failing job as well.

Could someone with write access re-run the failed job?