Repository navigation
Conversation
sauwming
left a comment
There was a problem hiding this comment.
Thanks for this — late SDP / offerless INVITE support is a real gap and the negotiation core here is going in the right direction. I'd like to see it land, but there are a few things to sort out first, mostly around the new on_call_send_ack callback.
The main concern is structural. As designed, on_call_send_ack converts a framework-guaranteed action into an application obligation with a silent no-op path. That's why three separate defects cluster on it (comments 1-3 below): the PJSUA2 binding installs it for every application, the handler returns without sending when the Call lookup fails, and the default implementation can throw a C++ exception through C frames while the dialog lock is held. Net effect is that an existing PJSUA2 application that never asked for this can now fail to ACK a 200/OK.
Two ways out, either is fine by me:
- Make the callback advisory — the application supplies the answer SDP (or a modified tdata) and the framework always sends the ACK. This removes the whole class of failure.
- Keep the current shape, but guarantee the fallback send on every path and wrap the PJSUA2 invocation in try/catch.
If you'd like this to move faster, splitting it would also work: the late-SDP negotiation core in this PR, and on_call_send_ack in a follow-up where the API can be discussed on its own.
Blocking before merge: comments 1-5 (the ACK regression, the exception escape, the ignored reject code) and comment 10 — on_call_send_ack is inserted in the middle of pjsua_callback, which shifts the offsets of every member after it. That's an ABI break for anything linked against an older shared pjsua, and it's free to avoid by appending at the end of the struct.
Would like addressed: comments 6-8. Follow-ups, not blocking: comments 9, 11, 12.
Both sip_inv.c and pjsua_call.c build clean with -Wall here. Nice to see the SIPp scenarios included for both UAC and UAS.
| = &Endpoint::on_call_tsx_terminate_session; | ||
| ua_cfg.cb.on_call_media_state = &Endpoint::on_call_media_state; | ||
| ua_cfg.cb.on_call_sdp_created = &Endpoint::on_call_sdp_created; | ||
| ua_cfg.cb.on_call_send_ack = &Endpoint::on_call_send_ack; |
There was a problem hiding this comment.
1. This installs the callback for every PJSUA2 application.
ua_cfg.cb.on_call_send_ack is set unconditionally, so pjsua_call_subsys_init() sets inv_cb.on_send_ack, and inv_send_ack() then takes the manual branch for all 2xx ACKs in every PJSUA2 app — not just ones that override onCallSendAck().
On its own that would be harmless if the default always sent the ACK, but combined with the NULL-Call early return below it silently changes ACK behaviour for existing applications. Either install it only when the application actually overrides the callback, or guarantee the default always sends.
There was a problem hiding this comment.
Code was changed to guarantee to send ACK
| pjsip_rx_data *rdata) | ||
| { | ||
| Call *call = Call::lookup(call_id); | ||
| if (!call) { |
There was a problem hiding this comment.
2. ACK is never sent when the lookup fails.
Call::lookup() returns NULL whenever user_data isn't set — the application destroyed the Call before the 200/OK arrived, or this is a transfer/replace call whose Call isn't attached yet. We return here without sending anything, and nothing downstream compensates.
Consequence: inv stays in CONNECTING, the peer retransmits the 200/OK until it times out, and any subsequent BYE is parked indefinitely by the "Delaying BYE request until ACK is received" path in pjsip_inv_send_msg(). Before this PR the framework always sent the ACK.
Suggest falling back rather than returning:
Call *call = Call::lookup(call_id);
if (!call) {
pjsua_call_send_ack(call_id, rdata->msg_info.cseq->cseq, NULL);
return;
}| { | ||
| pjsua_call *call = (pjsua_call*) inv->dlg->mod_data[pjsua_var.mod.id]; | ||
|
|
||
| if (!call) |
There was a problem hiding this comment.
3. Same early return at the C level.
Narrower than the PJSUA2 case — the else branch below does fall back to pjsua_call_send_ack() when no application callback is set — but if dlg->mod_data[pjsua_var.mod.id] has been cleared we still return without ACKing. Worth handling here too, since rdata alone is enough to send it.
| * | ||
| * @param prm Callback parameter. | ||
| */ | ||
| virtual void onCallSendAck(OnCallSendAckParam &prm) |
There was a problem hiding this comment.
4. The default implementation can throw through C frames.
sendAck() raises Error via PJSUA2_CHECK_RAISE_ERROR2, and Endpoint::on_call_send_ack() has no try/catch (the only catch in endpoint.cpp is at line 700). The exception therefore unwinds through inv_send_ack -> mod_inv_on_tsx_state -> the tsx/dialog callbacks — C frames, with the dialog lock held — giving std::terminate or at best a leaked lock.
This is reachable in normal operation: acquire_call() inside pjsua_call_send_ack() legitimately fails with PJ_ETIMEDOUT or PJSIP_ESESSIONTERMINATED. Every other pjsua2 default callback is a no-op and cannot throw, so the surrounding code isn't written to expect this. Please wrap the invocation in Endpoint::on_call_send_ack().
There was a problem hiding this comment.
Default implementation does not throw.
| } | ||
| } | ||
|
|
||
| call->opt = opt; |
There was a problem hiding this comment.
5. The application's rejection code is ignored.
code is passed to on_call_rx_reinvite() above but never checked here, unlike the existing paths at lines 6093 and 6108 which do if (code != PJSIP_SC_OK) goto on_return;.
So an application that sets code = 488 or 491 to reject an offerless re-INVITE is overridden: we fall through to !PJ_SUCCESS, sip_inv generates a 200/OK with a fresh offer, and call->opt = opt is applied even for the request the application just rejected.
There was a problem hiding this comment.
Code was changed to respect code from callback.
| */ | ||
| if (call->med_ch_cb || !call->inv->last_answer) { | ||
| if (call->med_ch_cb || | ||
| (call->inv->state < PJSIP_INV_STATE_CONFIRMED && |
There was a problem hiding this comment.
8. This can hit an assertion in debug builds.
Narrowing the condition to state < PJSIP_INV_STATE_CONFIRMED means the CONFIRMED + last_answer == NULL case no longer defers — it falls through to pjsip_inv_answer(), which opens with PJ_ASSERT_RETURN(inv->last_answer, PJ_EINVALIDOP). In any build without NDEBUG that's an abort().
That state is exactly what an application hits calling pjsua_call_answer() from inside on_call_rx_reinvite() — invite_tsx is the UAS re-INVITE in TRYING with no 100 sent yet — which is the workflow this PR is encouraging. Either keep deferring in that case, or return PJ_EINVALIDOP explicitly before reaching pjsip_inv_answer().
| if (status != PJ_SUCCESS) | ||
| goto on_return; | ||
|
|
||
| if (sdp) { |
There was a problem hiding this comment.
9. Two guards missing in pjsua_call_send_ack().
call->inv is passed to pjsip_inv_set_sdp_answer()/pjsip_inv_create_ack() without a NULL check — acquire_call() succeeds when only call->async_call.dlg is set, so call->inv can legitimately be NULL here.
Separately: if the 2xx carried no offer the negotiator is in DONE, and pjmedia_sdp_neg_set_local_answer() asserts state == REMOTE_OFFER || WAIT_NEGO. An application that passes an SDP in that situation gets an abort() in a debug build instead of PJMEDIA_SDPNEG_EINSTATE back. Worth validating the negotiator state before calling it.
| pjmedia_sdp_neg_get_state(inv->neg) == PJMEDIA_SDP_NEG_STATE_DONE) | ||
| { | ||
| struct tsx_inv_data *tsx_inv_data; | ||
| pjmedia_sdp_session *offer = (pjmedia_sdp_session*)local_sdp; |
There was a problem hiding this comment.
10. Stale offer is re-sent when on_create_offer() fails.
Two things here. The cast discards const on a caller-owned parameter, and on_create_offer() implementations (pjsua_call_on_create_offer()) may leave *offer untouched when they fail.
When that happens we fall through to pjmedia_sdp_neg_send_local_offer() below and re-offer the stale active SDP, even though media re-initialisation just failed. A failure in the callback should produce an error response rather than an offer the application can't back.
There was a problem hiding this comment.
Instead sending stale sdp offer PJ_EINVALIDOP is returned when on_create_offer() will not return sdp.
| if (status == PJ_SUCCESS && !local_offer) | ||
| status = pjmedia_sdp_neg_get_neg_local(inv->neg, &local_offer); | ||
| if (status == PJ_SUCCESS && local_offer) | ||
| last_res->msg->body = create_sdp_body(last_res->pool, local_offer); |
There was a problem hiding this comment.
11. This assignment is dead.
process_answer() runs immediately after (line 3175) with the negotiator in LOCAL_OFFER. It takes the neg_state == PJMEDIA_SDP_NEG_STATE_LOCAL_OFFER branch, retrieves the same SDP via pjmedia_sdp_neg_get_neg_local(), and unconditionally does tdata->msg->body = create_sdp_body(tdata->pool, sdp) — overwriting what's set here.
Harmless beyond a redundant clone into the tdata pool, but it obscures the intent. Not blocking.
| pjsip_tx_data_dec_ref(response); | ||
| PJ_PERROR(3, (THIS_FILE, status, | ||
| "Failed to create initial answer")); | ||
| return status; |
There was a problem hiding this comment.
12. The application isn't told that the async answer failed.
Returning non-PJ_SUCCESS here tells sip_inv "the application will not answer", so it generates its own 200/OK. But the application still believes it owns the answer, and its later pjsua_call_answer() finds the transaction already completed and fails with PJ_EINVALIDOP and no indication why.
Worth clearing the async intent or notifying the application on this path. Same for the pjsip_inv_send_msg() failure below. Not blocking.
There was a problem hiding this comment.
When fails sending 100 in async ACK application is not notified, but transaction is terminated and framework will not send 200 automaticaly.
|
CI failure is legit, it was C90 compatibility check which is only enabled in that CI job, here is the failure: |
823873c to
87e51a3
Compare
sauwming
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — I went through a7018a2c5 and 87e51a3ab. All twelve points from the previous review are genuinely addressed: the ACK fallback exists on both the C and PJSUA2 paths, the reject code is honoured, on_call_send_ack moved to the end of pjsua_callback so the ABI is fine, the sip_inv.c multiple-offer bypass and the answer-deferral change were dropped, the dead body assignment is gone, and pjsua_call_send_ack() grew its guards. The C90 build failure is fixed too — I compiled both changed C files with -std=gnu89 -Werror=declaration-after-statement -Wall and they're clean.
Unfortunately two of the fixes overshot, so I don't think it's mergeable yet. Both are in the new code rather than pre-existing.
The fallback ACK now inverts the lock order (comment 1). pjsua_call_on_send_ack() runs with the dialog lock held, and the fallback goes through pjsua_call_send_ack() → acquire_call(), which takes PJSUA_TRY_LOCK() afterwards. It try-locks so there's no hard deadlock, but under contention it spins for 2 s on the transport thread and then drops the ACK — which is the failure the fallback was added to prevent. The !call branch immediately above already does the right thing by calling call_inv_send_ack(inv, ...) directly.
PJ_EINVALIDOP leaves the re-INVITE unanswered (comment 2). Refusing to re-offer the stale SDP was the right call, but returning an error from pjsip_inv_answer() means no final response is ever sent, and pjsua_call_on_create_offer() yields a NULL offer for entirely ordinary reasons (hanging_up, media changing). The transaction then sits in PROCEEDING until the peer's Timer C fires.
Beyond those: the PJSUA2 hook is still installed for every application (comment 3), which is what routes every app into comment 1; suppressSendAck's documentation is copy-pasted from a terminate-session parameter, and following it puts a duplicate ACK on the wire (comment 4); inv->neg is dereferenced without a NULL check (comment 5); and call_inv_send_ack is PJ_DEF-exported with no prototype and no module prefix (comment 6).
Test coverage is the thing I'd most like to see move. pjsua_app.c has no reference to on_call_send_ack, so inv_cb.on_send_ack is never installed and none of the four new SIPp scenarios reach the new code — on_call_send_ack, pjsua_call_send_ack(), Call::sendAck(), the offerless branch of pjsua_call_on_rx_reinvite() and the new sip_inv.c block all ship untested, and that's exactly where comments 1-5 live. An offerless re-INVITE scenario covering sync accept, async accept, and reject-with-488 would cover most of it.
One documentation item I couldn't attach inline because the lines aren't in the diff: pjsua.h around line 1780. on_call_rx_reinvite() is now invoked with offer == NULL, and while the block already tells applications to "be prepared to handle the case of re-INVITE without SDP" (so this isn't an unannounced break), the sentence "Currently, this callback is only called for re-INVITE with SDP" is now false, and @param offer should say it may be NULL for an offerless re-INVITE. Same for OnCallRxReinviteParam::offer being empty in that case. Worth a release-note line as well, since a C application that dereferences offer unconditionally will crash on the first offerless re-INVITE.
Minor and non-blocking: local_offer in the new sip_inv.c block is written but never read; tsx_inv_data->has_sdp is initialised but never set to PJ_TRUE after the offer is installed (harmless today, the neg_state == DONE guard re-entry-protects the block); and the alt-pjsua-uas-late-offer*.py headers say "Run with standard pjsua" though both use --custom-sdp / --dummy-codecs, which only exist when PJSUA_MEDIA_HAS_PJMEDIA=0.
Blocking: comments 1-6. Would like addressed: 7 and the test coverage. Follow-ups: 8-10.
| skip_sending_ack = (*pjsua_var.ua_cfg.cb.on_call_send_ack)(call->index, rdata); | ||
| } | ||
| if (!skip_sending_ack) { | ||
| pjsua_call_send_ack(call->index, rdata->msg_info.cseq->cseq, NULL); |
There was a problem hiding this comment.
1. This re-acquires the call lock while the dialog lock is held.
pjsua_call_on_send_ack() is reached via inv_send_ack() <- mod_inv_on_tsx_state <- pjsip_dlg_on_tsx_state, so the dialog lock is held here. pjsua_call_send_ack() then calls acquire_call(), which takes PJSUA_TRY_LOCK() — the inverse of the documented PJSUA_LOCK > dialog ordering.
Because it try-locks there's no hard deadlock, but under contention it spins for PJSUA_ACQUIRE_CALL_TIMEOUT (2 s) on the transport/ioqueue thread, then returns PJ_ETIMEDOUT and the ACK is silently not sent. inv stays in CONNECTING, the peer retransmits the 200/OK, and any BYE is parked by "Delaying BYE request until ACK is received" — the exact failure this fallback was added to prevent.
inv is already in hand, and the !call branch four lines above already does it correctly:
if (!skip_sending_ack) {
call_inv_send_ack(inv, rdata->msg_info.cseq->cseq, NULL);
}The same concern applies to the public pjsua_call_send_ack() when an application calls it from inside the callback, which is the documented usage — worth a note in its Doxygen that it must not be called from on_call_send_ack(), or better, making it safe to do so.
There was a problem hiding this comment.
Changed pjsua_call_send_ack() to call_inv_send_ack() and solved that pjsua_call_send_ack() can be called synchronously from on_call_send_ack() callback.
| /* App callback was invoked but did not provide a usable | ||
| * offer; do not silently re-offer the stale active SDP. | ||
| */ | ||
| status = PJ_EINVALIDOP; |
There was a problem hiding this comment.
2. This leaves the re-INVITE permanently unanswered.
Not re-offering the stale active SDP is right, but returning an error aborts pjsip_inv_answer() without producing any final response.
pjsua_call_on_create_offer() leaves *offer NULL for ordinary reasons — call->hanging_up, or media in the middle of changing — so this isn't an exotic path. In the async flow a 100 has already gone out, the application gets an error back, and the UAS never sends a final response: the transaction sits in PROCEEDING until the peer's Timer C fires.
The pre-existing synchronous path for exactly this situation (around line 6421) falls back to pjmedia_sdp_neg_send_local_offer(). Either match that, or answer with a 5xx so the transaction completes — but the request has to be answered one way or the other.
There was a problem hiding this comment.
When no offer is returned from pjsua_call_on_create_offer() than 500 is responded.
| = &Endpoint::on_call_tsx_terminate_session; | ||
| ua_cfg.cb.on_call_media_state = &Endpoint::on_call_media_state; | ||
| ua_cfg.cb.on_call_sdp_created = &Endpoint::on_call_sdp_created; | ||
| ua_cfg.cb.on_call_send_ack = &Endpoint::on_call_send_ack; |
There was a problem hiding this comment.
3. Still installed for every PJSUA2 application.
The default is safe in isolation now, but nothing here lets an application that never overrides onCallSendAck() opt out. Every PJSUA2 app's 2xx ACKs therefore leave the framework path in inv_send_ack() and go through the wrapper in comment 1.
Installing the hook lazily — only when the application actually overrides the callback — removes this and comment 1 together, and keeps the default behaviour bit-identical to today's for every existing app.
There was a problem hiding this comment.
Added new configuration option UaConfig.enableCallSendAckCallback for PJSUA2 which enables onSendAck() callback. Default is that this callback is disabled.
| * session termination and keep the call alive. Default is false, i.e. | ||
| * the library will terminate the call as per default behavior. | ||
| */ | ||
| bool suppressSendAck; |
There was a problem hiding this comment.
4. This documentation belongs to a different callback, and following it sends two ACKs.
"suppress the automatic session termination and keep the call alive ... the library will terminate the call as per default behavior" is copy-pasted from a terminate-session parameter. The field actually suppresses the automatic ACK.
The practical consequence: an application that reads this will call sendAck() with its own SDP answer and leave suppressSendAck false, believing the flag concerns call termination. pjsua_call_on_send_ack() then sees skip_sending_ack == false and sends a second ACK — pjsip_inv_create_ack() dec-refs the one just sent and puts a duplicate on the wire.
Suggest something like: "Output: application sets this to true if it has sent (or will send) the ACK itself, to suppress the automatic ACK sending. Default is false, i.e. the library sends the ACK after this callback returns."
| return PJ_EINVAL; | ||
|
|
||
| if (sdp) { | ||
| neg_state = pjmedia_sdp_neg_get_state(inv->neg); |
There was a problem hiding this comment.
5. inv->neg can be NULL here.
inv is checked above, but pjmedia_sdp_neg_get_state() opens with PJ_ASSERT_RETURN(neg != NULL, ...), so this aborts in any build without NDEBUG.
Reachable through the public API: after an offerless INVITE whose 2xx also carried no SDP, inv->neg is still NULL. An application calling pjsua_call_send_ack(id, cseq, sdp) there gets an abort() rather than an error return.
if (inv->neg == NULL)
return PJMEDIA_SDPNEG_EINSTATE;There was a problem hiding this comment.
Added test for inv->neg == NULL
| return pjsua_call_answer2(call_id, opt, code, reason, msg_data); | ||
| } | ||
|
|
||
| PJ_DEF(pj_status_t) call_inv_send_ack(pjsip_inv_session *inv, |
There was a problem hiding this comment.
6. This should be static.
PJ_DEF gives it external linkage, so libpjsua exports a bare call_inv_send_ack symbol with no declaration in any header and no pjsua_ prefix — both a collision risk with application code and a departure from the module-prefix convention.
It's only used within this file (here and in pjsua_call_on_send_ack()), so plain static pj_status_t call_inv_send_ack(...) is enough.
There was a problem hiding this comment.
call_inv_send_ack() is static now
| @@ -0,0 +1,38 @@ | |||
| # | |||
There was a problem hiding this comment.
7. None of the four new scenarios exercise the new code.
Good to see SIPp scenarios included, but as written they cover the pre-existing paths:
- The UAC scenarios use the framework's existing answer-in-ACK path.
pjsua_app.cnever setson_call_send_ack, soinv_cb.on_send_ackis never installed andpjsua_call_on_send_ack()is never reached. - The UAS scenarios cover an initial offerless INVITE, which never reaches the new block in
pjsip_inv_answer()— that requiresinv->state == PJSIP_INV_STATE_CONFIRMED.
So on_call_send_ack, pjsua_call_send_ack(), Call::sendAck(), the offerless branch of pjsua_call_on_rx_reinvite() and the new sip_inv.c block all ship with no coverage — which is where comments 1-5 live.
An offerless re-INVITE scenario is the one that would pay off: sync accept, async accept, and reject with 488. That last one would have caught the ignored code from the previous round.
| * | ||
| * @param prm Callback parameter. | ||
| */ | ||
| virtual void onCallSendAck(OnCallSendAckParam &prm) |
There was a problem hiding this comment.
8. This description doesn't match the implementation.
"The default implementation simply sends the ACK without any SDP answer, using sendAck()" — but the default body is { PJ_UNUSED_ARG(prm); }, and the send happens down in the C layer via pjsua_call_on_send_ack().
The behaviour is right; only the text is wrong. Combined with comment 4, though, an application author reading this header can't work out who owns the ACK — and an override that calls the base implementation expecting it to send will get nothing.
| * request. If its wholeSdp is empty, no SDP body | ||
| * will be attached. | ||
| */ | ||
| void sendAck(int cseq, const SdpSession &sdp = SdpSession()) |
There was a problem hiding this comment.
9. The cseq isn't reachable from PJSUA2.
OnCallSendAckParam carries only rdata, so an application overriding onCallSendAck() has to reach through the abstraction to get the CSeq:
((pjsip_rx_data*)prm.rdata.pjRxData)->msg_info.cseq->cseqThat's the one piece of raw PJSIP a pjsua2 user shouldn't need. Either add cseq to OnCallSendAckParam, or give sendAck() a default that derives it from the call's pending 2xx. Non-blocking, but worth settling before the API ships, since changing it later is a break.
There was a problem hiding this comment.
Added cseq into OnCallSendAckParam
| { "server-affinity", 2, 0, OPT_SERVER_AFFINITY}, | ||
| #if !PJSUA_MEDIA_HAS_PJMEDIA | ||
| { "custom-sdp", 1, 0, OPT_CUSTOM_SDP}, | ||
| { "dummy-codecs", 0, 0, OPT_DUMMY_CODECS}, |
There was a problem hiding this comment.
10. --dummy-codecs isn't registered anywhere else.
It's in the option table and parsed at line 1733, but absent from usage() (where --custom-sdp appears, line 188) and from the settings serializer near line 2802.
So it doesn't show up in --help, and it's silently dropped across an app restart or a config dump-and-reload.
13cc3ca to
b04cd28
Compare
sauwming
left a comment
There was a problem hiding this comment.
Round 3 on b04cd2830. The structural fixes from round 2 all landed correctly: the lock inversion is gone via the in-callback guard, PJ_EINVALIDOP is replaced by a real response, the PJSUA2 hook is opt-in through UaConfig::enableCallSendAckCallback, call_inv_send_ack() is static, and the ABI is preserved. A local CMake Debug build with -Wall is clean, and both changed C files pass -std=gnu89 -Werror=declaration-after-statement -Wall.
Still not mergeable, unfortunately — one fix introduced a new way to drop the ACK, and there's an uninitialized-pointer issue in the offerless re-INVITE branch.
Comment 1 is my fault, and I'm sorry for the churn. My round-2 note gave the inv->neg == NULL guard as a bare snippet without saying where it belonged, and placed at the top of the function it now swallows the sdp == NULL case — which is exactly the guaranteed-fallback path it was meant to protect. Moving it inside if (sdp) is the whole fix.
Comment 2 is the one I'd look at hardest. pjsip_tx_data *response; is uninitialized, and pjsip_inv_initial_answer() doesn't set *p_tdata on most of its failure paths — so if (response) pjsip_tx_data_dec_ref(response) reads an indeterminate pointer. And on the one failure path where it does set it (session-timer rejection, which hands back a valid 422 Session Interval Too Small), the new code dec-refs that response and calls pjsip_inv_terminate(inv, 500, PJ_TRUE) — so a re-INVITE that should be answered 422 tears the whole call down. The pattern is copied from the pre-existing block at line 6129, which has the same latent issue, but these are two new instances.
Two items from round 2 are still open: the on_call_rx_reinvite() offer-may-be-NULL documentation, and test coverage.
On the docs — I couldn't anchor this inline because the lines aren't in the diff: pjsua.h around line 1779 still says "Currently, this callback is only called for re-INVITE with SDP", and @param offer is still plain "Remote offer." The block does already advise applications to be prepared for a re-INVITE without SDP, so this isn't an unannounced break, but the sentence is now false and the parameter needs the NULL note. Same for OnCallRxReinviteParam::offer, plus a release-note line.
On coverage (comment 8) — on_call_send_ack still has zero references anywhere under pjsip-apps/, so inv_cb.on_send_ack is never installed by any test binary, and none of the four SIPp scenarios covers an offerless re-INVITE. Every HIGH and MEDIUM finding in this round lives in code that no test executes. I'd rather not keep finding these by inspection: three rounds in a row have each introduced a new defect in the untested region, and an offerless re-INVITE scenario with sync-accept, async-accept and reject-with-488 would have caught comments 1, 2, 4 and the round-2 code regression.
Process blocker: CI has never run against this head. All six GitHub Actions workflows on b04cd2830 are sitting in action_required awaiting maintainer approval, and ci/bitrise/e4b6aade20ea9eb3/pr is currently failing. I'll get the workflows approved — the four scenarios this PR adds to ci-linux.yml and the C90 job that caught the round-1 breakage have never executed here.
Also worth folding into the next push: per the repo convention, the Teluu copyright end year should be bumped to 2026 in the modified files (pjsua_call.c, sip_inv.c, pjsua_core.c, call.hpp, endpoint.hpp, call.cpp, endpoint.cpp, pjsua_app_config.c) — only pjsua.h is current.
One API gap that isn't a defect but will bite users: pjsua_call_answer_with_sdp(), which the on_call_rx_reinvite() docs point applications to for async answers, calls pjsip_inv_set_sdp_answer() and so fails with PJMEDIA_SDPNEG_EINSTATE when the negotiator is in DONE. It therefore can't be used to answer an offerless re-INVITE at all — the application is implicitly forced through on_create_offer/on_call_sdp_created. Worth either making it work or documenting.
Blocking: comments 1-5. Would like addressed: 6, 7, and the coverage gap. Minor: 9, 10.
Non-blocking leftovers: local_offer (sip_inv.c:3131) is written and never read; tsx_inv_data->has_sdp is never set PJ_TRUE after the offer is installed (harmless only because the neg_state == DONE guard re-entry-protects the block); two new lines exceed 80 columns (pjsua_call.c:3380, 7594).
| if (inv == NULL) | ||
| return PJ_EINVAL; | ||
|
|
||
| if (inv->neg == NULL) |
There was a problem hiding this comment.
1. This guard is in the wrong place, and it defeats the fallback ACK.
My fault — I gave this as a bare snippet last round without saying where it went. Sitting here, before if (sdp), it returns early for the sdp == NULL case too, which is precisely the guaranteed-fallback path.
Concretely: an application installs on_call_send_ack, we send an offerless INVITE, and the peer answers 200 OK with no SDP body. inv->neg was never created, so this returns PJMEDIA_SDPNEG_EINSTATE before doing anything. inv_send_ack() (sip_inv.c:650) has already handed the entire ACK over to the callback and returned PJ_SUCCESS, so nothing else sends it — no ACK on the wire, peer retransmits the 200, call torn down.
It only guards the negotiator access, so it belongs with the other negotiator check:
if (sdp) {
if (inv->neg == NULL)
return PJMEDIA_SDPNEG_EINSTATE;
neg_state = pjmedia_sdp_neg_get_state(inv->neg);
...
}| NULL, &async, &code, | ||
| &opt); | ||
| if (async) { | ||
| pjsip_tx_data *response; |
There was a problem hiding this comment.
2. response is uninitialized here, and the 422 path tears down the call.
pjsip_inv_initial_answer() does not set *p_tdata on its pjsip_dlg_create_response() failure (sip_inv.c:3015) or its process_answer() failure (sip_inv.c:3045) — it just goto on_return. So if (response) pjsip_tx_data_dec_ref(response); reads an indeterminate pointer and can dec-ref garbage. pjsip_tx_data *response = NULL; fixes that part.
The worse case is the path where *p_tdata is set. On session-timer rejection (sip_inv.c:3031) pjsip_timer_process_req() fails but hands back a valid 422 Session Interval Too Small in *p_tdata, with a non-PJ_SUCCESS return. This code then dec-refs that 422 and calls pjsip_inv_terminate(inv, 500, PJ_TRUE) — so a re-INVITE that should be answered 422 (and renegotiated by the peer) kills the call instead.
Suggest distinguishing the two: if response is non-NULL, send it rather than discarding it; only terminate when there's genuinely nothing to send. Second instance at line 6311, and note the pre-existing block at 6129 has the same shape if you want to fix all three together.
| } | ||
| } | ||
| if (!skip_sending_ack) { | ||
| call_inv_send_ack(inv, rdata->msg_info.cseq->cseq, NULL); |
There was a problem hiding this comment.
3. The fallback's return value is dropped.
Both call sites (here and line 7587) ignore the result of call_inv_send_ack(). When comment 1 fires — or pjsip_inv_create_ack() fails for any other reason — the ACK is silently never sent, with no retry and no signal upward; the only trace is the pjsua_perror inside the callee.
This is the failure the fallback exists to prevent, so it shouldn't be the quiet path. At minimum log at warning level with the call id; better, retry once without the SDP, since a bad answer SDP is the most likely cause.
| /* App callback was invoked but did not provide a usable | ||
| * offer; do not silently re-offer the stale active SDP. | ||
| */ | ||
| st_code = PJSIP_SC_INTERNAL_SERVER_ERROR; |
There was a problem hiding this comment.
4. Sync and async now answer the same condition differently.
Replacing PJ_EINVALIDOP with a real response was the right move. But the synchronous path for this exact situation — on_create_offer() yielding no offer — falls back to pjmedia_sdp_neg_send_local_offer() at line 6480 and answers 200 with the active SDP, while this path answers 500.
pjsua_call_on_create_offer() leaves *offer NULL for ordinary reasons (call->hanging_up, media changing), so an offerless re-INVITE now gets 200 or 500 depending only on whether the application set async. That's hard to explain to an application author, and the sync behaviour is the "stale re-offer" the comment just above says to avoid.
Worth picking one. If the stale re-offer is wrong, it's wrong in both paths; if 500 is right, the sync path should match.
| * Notify application when the framework needs to send an ACK request | ||
| * after it receives an incoming 2xx response for INVITE (e.g. when the | ||
| * 2xx response carries a late SDP offer). If this callback is | ||
| * implemented, the application is responsible for creating and sending |
There was a problem hiding this comment.
5. This prose contradicts the @return below it.
"If this callback is implemented, the application is responsible for creating and sending the ACK request itself" vs. "PJ_FALSE (default) to keep current behavior, automatic ACK sending by the framework will proceed as usual."
Both can't hold. An application that follows the prose — calls pjsua_call_send_ack() itself and returns PJ_FALSE, or simply falls off the end of a handler returning 0 — puts two ACKs for the same CSeq on the wire.
The @return describes the actual behaviour, so the prose is what needs changing: implementing the callback doesn't transfer responsibility, returning PJ_TRUE does. The equivalent PJSUA2 suppressSendAck text was fixed last round; this one was missed.
|
|
||
| /* An asynchronous confirmed UAS re-INVITE without an SDP offer is | ||
| * answered with a new local offer in the 2xx response. */ | ||
| if (st_code/100 == 2 && inv->state == PJSIP_INV_STATE_CONFIRMED && |
There was a problem hiding this comment.
6. This also catches re-INVITEs carrying a non-SDP body.
The block keys off has_sdp being false, but that's also false for a body that isn't SDP. With PJSIP_INV_ACCEPT_UNKNOWN_BODY, inv_check_sdp_in_incoming_msg() returns before creating tsx_inv_data, so a re-INVITE carrying e.g. application/isup now gets a 200 OK with a fresh SDP offer instead of a bodyless 200.
Checking that the request genuinely had no body, rather than no SDP body, would keep this to the intended case.
| @@ -0,0 +1,38 @@ | |||
| # | |||
There was a problem hiding this comment.
7. The new code still has no test coverage.
Raised last round and unchanged, so restating it once with specifics:
on_call_send_ackhas zero references anywhere underpjsip-apps/, soinv_cb.on_send_ackis never installed by any test binary.pjsua_call_send_ack(),pjsua_call_on_send_ack(), the new in-callback guard andCall::sendAck()are all unexecuted.- None of the four scenarios covers an offerless re-INVITE, so the new
pjsua_call_on_rx_reinvite()branch and the newsip_inv.cblock are unexecuted too.
That's where comments 1, 2, 4 and 6 live, and where the previous round's ignored-code regression lived. A single offerless re-INVITE scenario — sync accept, async accept, reject with 488 — would cover almost all of it, and would have caught each of those by itself.
| @@ -0,0 +1,33 @@ | |||
| # | |||
| # Test driver for alt-pjsua-uas-late-offer.xml | |||
There was a problem hiding this comment.
8. Wrong XML named here, and both headers name the wrong binary.
This line points at alt-pjsua-uas-late-offer.xml rather than this scenario's own alt-pjsua-uas-late-offer-dummy-codecs.xml.
Separately, both UAS drivers say "Run with standard pjsua" in their headers, but they pass --custom-sdp / --dummy-codecs, which only exist when PJSUA_MEDIA_HAS_PJMEDIA=0. Anyone following the header gets an unknown-option error.
b04cd28 to
e25be18
Compare
sauwming
left a comment
There was a problem hiding this comment.
Round 4 on e25be18c2. Every round-3 blocker is resolved: the inv->neg guard is inside if (sdp), the uninitialized response / 422-teardown code is gone, the fallback ACK's return value is logged, sync and async offerless re-INVITE now both fall back to pjmedia_sdp_neg_send_local_offer(), the on_call_send_ack prose matches its @return, and has_body correctly separates a non-SDP body from an offerless request.
The coverage gap is genuinely closed, and that's the big one. pjsua_late_sdp_offer_test exercises on_call_send_ack, pjsua_call_send_ack(), the offerless re-INVITE branch and the new sip_inv.c block across sync/async answer × sync/async ACK, and inv_offer_answer_test covers the NULL-application-offer and unknown-body cases. Thank you for doing that properly rather than minimally — it's what makes this reviewable. Local verification: CMake Debug build with -Wall clean, -std=gnu89 -Werror=declaration-after-statement -Wall clean on all five changed C files, and the four affected test suites pass when run together.
Bitrise is green now too. I've re-approved the GitHub Actions workflows — they were reset to action_required by the force-push, so ci-linux and the C90 job still haven't run on this head.
I think one more round, mainly for comment 1.
Comment 1 is a debug-build abort() reachable through the API this PR widens. The new if (call->answer_sdp) branch skips pjsua_media_channel_create_sdp(), which is what normally syncs call->med_prov_cnt to the local SDP's media count — and the grow path that would compensate is gated on app_managed, which no longer holds now that ordinary calls can supply their own offer.
Comment 2 is round-1 comment 4 again. The reply then was that the default implementation doesn't throw, which is true, but it's an override that throws — and Call::sendAck() is the documented way to use this callback. Worth closing properly this time.
Smaller repo-convention items for the next push, not worth inline comments:
git diff --checkreports trailing whitespace on five added lines:call.hpp:853, 863, 2185, 2186andpjsua_call.c:3640.- The new
--dummy-codecsusage line (pjsua_app_config.c:203) is 93 columns against the project's ~80. - Teluu copyright end year still
2008-2011in four touched files:pjsip/src/test/inv_offer_answer_test.c,pjsip/src/test/test.c,pjsip/src/test/test.h, andpjsip-apps/src/3rdparty_media_sample/alt_pjsua_aud.c. Everything else was updated to 2026.
One layering observation, non-blocking and your call entirely: alt_pjsua_aud.c now reads the pjsua app's app_config global via #include "../pjsua/pjsua_app.h". That file is the reference implementation a third-party media stack is meant to copy, and it's a pjsua-lib hook rather than app code, so the include makes it uncompilable outside the alt_pjsua build. Ordering is safe (load_config() runs before pjsua_init()), so this is purely about what the sample teaches.
Blocking: comments 1 and 2. Would like addressed: 3 and 4. Minor: 5, 6.
| if (status != PJ_SUCCESS) { | ||
| pjsua_perror(THIS_FILE, "Error creating SDP answer", status); | ||
| goto on_return; | ||
| if (call->answer_sdp) { |
There was a problem hiding this comment.
1. An application-supplied offer bypasses med_prov_cnt sync, and asserts in debug builds.
Taking call->answer_sdp here skips pjsua_media_channel_create_sdp(), which is what normally brings call->med_prov_cnt in line with the local SDP's media count.
Concrete path, on an ordinary (non-app-managed) account with the default opt.aud_cnt=1, vid_cnt=0:
- Incoming offerless INVITE.
- App calls
pjsua_call_answer_with_sdp(id, sdp, NULL, 200, ...)with an audio+video SDP. pjsua_media_channel_init()has setmed_prov_cnt = 1; the offer goes out with 2 m-lines.- Peer answers both m-lines in the ACK.
pjsua_media_channel_update()reachespj_assert(call->med_prov_cnt >= local_sdp->media_count)(pjsua_media.c:4711) with1 >= 2→abort().
The grow path immediately above that assert (pjsua_media.c:4700) would handle it, but it's gated on app_managed, i.e. (call->opt.flag & PJSUA_CALL_MEDIA_APP_MANAGED) — which is exactly the assumption this PR invalidates, since an ordinary call can now supply its own offer. In release builds the assert vanishes and the extra m-line is silently dropped instead.
Either extend that grow path to cover an application-supplied local SDP, or validate up front in pjsua_call_answer_with_sdp() and reject with PJMEDIA_SDPNEG_EMISMEDIA when the SDP has more media lines than the call was initialised for. The second is probably kinder — it gives the application an error it can act on rather than a surprise at ACK time.
There was a problem hiding this comment.
Added validation of SDP for initial INVITE and check that number of media matches sdp.
| } | ||
| } | ||
|
|
||
| pj_bool_t Endpoint::on_call_send_ack(pjsua_call_id call_id, |
There was a problem hiding this comment.
2. An override of onCallSendAck() can still throw through C frames with the dialog lock held.
This is round-1 comment 4. The reply was that the default implementation doesn't throw, which is correct — but the risk is an override, and Call::sendAck() is the documented way to use this callback. It raises Error via PJSUA2_CHECK_RAISE_ERROR2.
That's reachable in ordinary operation, not just on exotic failures. inv_send_ack() (sip_inv.c:654) invokes the callback for every 2xx ACK, not only late-offer ones, so an application that always attaches an answer SDP will hit PJMEDIA_SDPNEG_EINSTATE on its very first normal offer-in-INVITE call — the negotiator is already DONE there. The exception then unwinds through pjsua_call_on_send_ack → inv_send_ack → mod_inv_on_tsx_state: C frames, dialog lock held, so std::terminate or a leaked lock.
try {
call->onCallSendAck(prm);
} catch (Error &err) {
PJ_LOG(1,(THIS_FILE, "onCallSendAck() error: %s", err.info().c_str()));
return PJ_FALSE;
}Returning PJ_FALSE keeps the guaranteed fallback ACK intact, so a throwing override degrades to "framework sends the ACK" rather than taking the process down.
| callback_inv = (pjsip_inv_session*) | ||
| pj_thread_local_get(ack_callback_inv_tls); | ||
|
|
||
| if (callback_inv) { |
There was a problem hiding this comment.
3. This rejects every call id except the one being ACKed.
When the TLS slot holds an inv session, the function requires call->index == call_id and otherwise returns PJ_EINVALIDOP.
The motivating use case in the on_call_send_ack documentation is 3PCC, where an application bridging two legs would quite naturally send leg B's ACK from inside leg A's on_call_send_ack() — and that now fails with a status that gives no hint as to why.
If the restriction is deliberate (avoiding the acquire_call() lock-order problem for the other leg), it should at least be documented on pjsua_call_send_ack(). If it isn't, falling through to the acquire_call() path when the ids differ would restore the 3PCC case, since the dialog lock held is leg A's, not leg B's.
There was a problem hiding this comment.
Added fallback to acquire_call() in case when call_id doesn't match.
| call = (pjsua_call*) inv->dlg->mod_data[pjsua_var.mod.id]; | ||
|
|
||
| if (!offer && pjsua_call_media_is_app_managed(call)) { | ||
| if (!offer && (pjsua_call_media_is_app_managed(call) || |
There was a problem hiding this comment.
4. This widens NULL-offer delivery to ordinary calls — needs a release note.
Adding || pjsua_var.ua_cfg.cb.on_call_rx_reinvite means an existing application that registered this callback, and until now only ever saw it with a non-NULL offer on a non-app-managed call, will get offer == NULL on the first offerless re-INVITE. One that dereferences offer unconditionally crashes.
The header documentation now covers this properly — thank you — but there's no release-note entry in the diff, and this is the kind of change that wants one.
Secondary effect in the same block worth a look: apply_call_setting(call, &opt, NULL) now runs on every accepted offerless re-INVITE for ordinary calls, where previously it did not. Benign as far as I can tell, but it's a behaviour change that isn't called out.
There was a problem hiding this comment.
apply_call_setting() is called only if opt is changed. I added release note into PR comment.
| status = pjsip_inv_set_local_sdp(inv, sdp); | ||
| } else { | ||
| status = pjsip_inv_set_sdp_answer(inv, sdp); | ||
| /* For an initial offerless INVITE, let answer2() initialize media before |
There was a problem hiding this comment.
5. This condition is missing !call->med_ch_cb.
It otherwise mirrors pjsua_call_answer2()'s media-init condition exactly, and I think the omission is unintentional.
If a media-channel init is already in flight, call->answer_sdp gets set here but on_answer_call_med_tp_complete() is not the pending completion callback — on_incoming_call_med_tp_complete() is, and it doesn't consume answer_sdp. The SDP then either gets picked up by a later unrelated answer or lingers until the call is reset.
Adding !call->med_ch_cb makes the two conditions identical, which is probably what was intended.
| * after it receives an incoming 2xx response for INVITE (e.g. when the | ||
| * 2xx response carries a late SDP offer). The default implementation | ||
| * simply do nothing and let's framework send the ACK without any SDP answer. | ||
| * Application can set prm.suppressSendAck to true and than the application |
There was a problem hiding this comment.
6. Wording in the new doc block.
"and than the application is responsible" → "and then". Two lines above, in the onCallSendAck() summary: "The default implementation simply do nothing and let's framework send the ACK" → "does nothing and lets the framework send the ACK".
These two lines and 853/863 in this file also carry trailing whitespace (git diff --check).
sauwming
left a comment
There was a problem hiding this comment.
Round 5 on 836b5a67. Local checks: CMake Debug build with -Wall clean, C90 -Wdeclaration-after-statement clean on the changed C files, git diff --check clean, and both inv_offer_answer_test and pjsua_late_sdp_offer_test pass.
First, CI results — this is the useful news. The previous head (e25be18c) did finally run a full GitHub Actions set after I approved it: 29 of 30 checks green. The one failure, "Default / pjmedia,pjsip", is not yours:
==7503==ERROR: AddressSanitizer: heap-use-after-free on address 0x5230000056a0
WRITE of size 8 at 0x5230000056a0 thread T0
#0 tcp_destroy ../src/pjsip/sip_transport_tcp.c:898
#1 tcp_destroy_transport ../src/pjsip/sip_transport_tcp.c:851
freed by thread T0 here:
#6 tcp_on_destroy ../src/pjsip/sip_transport_tcp.c:955
#11 pj_activesock_close ../src/pj/activesock.c:324
#12 tcp_destroy ../src/pjsip/sip_transport_tcp.c:897
SUMMARY: heap-use-after-free sip_transport_tcp.c:898 in tcp_destroy
That's a pre-existing TCP transport destroy-ordering bug — line 897 calls pj_activesock_close(), which drops the last group-lock reference and frees the transport's pool via tcp_on_destroy, and line 898 then writes to the freed struct. Nothing to do with late SDP. I'll raise it separately.
The catch is that it aborted pjsip-test before pjsua_call_test / pjsua_late_sdp_offer_test ran, so your new tests still haven't executed in CI — they pass locally, but CI hasn't confirmed them. I've approved the workflows on this head too.
Comment 3 is my mistake, and I'm sorry — that's twice now. Last round I suggested falling through to acquire_call() when the call ids differ, reasoning that the dialog lock held is leg A's rather than leg B's. That reasoning was wrong: the ordering constraint is PJSUA_LOCK > dialog lock, and acquire_call() takes the global PJSUA_TRY_LOCK() regardless of which dialog we're holding. So the fall-through reintroduces exactly the inversion I flagged in round 2. The PJ_EINVALIDOP you had before was safer; the real fix is a cross-call path that doesn't take PJSUA_LOCK, or documenting the limitation.
Comment 1 is the other half of round-4 comment 1. The media-count validation guards the initial-INVITE branch only; on a confirmed call the negotiator is DONE, so an offerless re-INVITE answered with an app SDP still reaches pjsip_inv_set_local_sdp() unchecked — and that's the flow the new on_call_rx_reinvite() docs advertise.
Comments 2 and 4 are narrower state-machine gaps in the same new function; 5 and 6 are small.
To be clear about where this stands: the feature itself looks right to me, the test coverage is real, and rounds 4 and 5 have been about a single function's edge cases rather than the design. I don't think you're far off.
Blocking: comments 1, 2, 3. Would like addressed: 4, 5. Documentation: 6.
| return status; | ||
| } | ||
|
|
||
| status = pjsip_inv_set_local_sdp(call->inv, sdp); |
There was a problem hiding this comment.
1. The offerless re-INVITE path still bypasses the new media-count validation.
pjsua_call_validate_initial_local_offer() only guards the branch above, whose entry condition requires !call->inv->neg || neg == NULL. On a confirmed call the negotiator is DONE, so an application-supplied offer lands here and goes straight to pjsip_inv_set_local_sdp() → pjmedia_sdp_neg_modify_local_offer2() with no check at all.
Same shape as round-4 comment 1, reached by the re-INVITE half:
- Ordinary (non-app-managed) call,
aud_cnt=1,med_prov_cnt == 1. - Peer sends an offerless re-INVITE.
- App sets
asyncinon_call_rx_reinvite()and answerspjsua_call_answer_with_sdp(id, audio+video sdp, NULL, 200, ...)— the flow the new docs advertise. - 200 goes out with 2 m-lines, peer answers both.
pjsua_media_channel_update()hitspj_assert(call->med_prov_cnt >= local_sdp->media_count)(pjsua_media.c:4711) with1 >= 2→ abort in debug, silently dropped m-line in release.
The grow path above that assert is still gated on app_managed. Calling the same validation here would close it, though the check needs to compare against the current med_prov_cnt rather than the initial call setting.
There was a problem hiding this comment.
Media count is checked for every SDP offer
| * setting the supplied SDP offer. | ||
| */ | ||
| if (code < 300 && sdp && !call->med_ch_cb && | ||
| (call->opt_inited || code == 183 || code / 100 == 2) && |
There was a problem hiding this comment.
2. Provisional codes other than 183 fall through into a bad state.
The gate (call->opt_inited || code == 183 || code / 100 == 2) lets e.g. code == 180 past when opt_inited is still false.
What then happens: the branch is skipped, so we reach pjsip_inv_set_local_sdp() at the bottom, which takes the inv->neg == NULL path and calls inv_create_neg_local_offer(). The negotiator is now built from the application's SDP while med_prov_cnt is still 0. A later pjsua_call_answer2(id, opt, 200, ...) skips its own media-init block (that requires !neg || neg == NULL, no longer true), so media is never initialised and the same med_prov_cnt assert fires at negotiation time.
Before this PR the else branch was pjsip_inv_set_sdp_answer(), which returned an error for this case rather than silently constructing the bad state — so this is a small regression in diagnosability as well.
Either widen the gate to all code < 200, or reject an SDP supplied with a non-183 provisional response.
There was a problem hiding this comment.
non reliable responses with SDP offer are denined
| } | ||
| } | ||
|
|
||
| if (!callback_inv) { |
There was a problem hiding this comment.
3. This fall-through reintroduces the round-2 lock inversion — my fault for suggesting it.
Last round I proposed falling through to acquire_call() when the ids differ, on the reasoning that the dialog lock held belongs to leg A rather than leg B. That was wrong. The documented ordering is PJSUA_LOCK > dialog grp_lock, and acquire_call() takes the global PJSUA_TRY_LOCK() — which dialog we're holding doesn't matter.
So in the 3PCC case named in the new on_call_send_ack docs — a bridge sending leg B's ACK from leg A's callback — we're on the transport/ioqueue thread holding leg A's dialog lock and then try-locking PJSUA_LOCK. Under contention that stalls the ioqueue thread for up to PJSUA_ACQUIRE_CALL_TIMEOUT (2 s) and then returns PJ_ETIMEDOUT with no ACK sent.
The PJ_EINVALIDOP you had in round 4 was the safer behaviour; I shouldn't have pushed you off it. Options as I see them now:
- Restore the rejection and document the same-call-id limitation on
pjsua_call_send_ack().pjsua.h:6996currently says "This function is safe to call synchronously from the on_call_send_ack() callback" with no caveat, so that line needs the qualifier either way. - Or look up the other call's
invwithout PJSUA_LOCK — the TLS slot already proves we're inside a callback, so apjsua_var.calls[call_id].invread under the dialog lock of leg A may be enough for the ACK path.
Your call which; I'd lean to the first, since it's honest about the constraint rather than papering over it.
There was a problem hiding this comment.
Only locked dialogue can be answered synchronously.
| pjsip_dlg_dec_lock(dlg); | ||
| return PJ_ENOMEM; | ||
| } | ||
| call->answer_sdp = pending_sdp; |
There was a problem hiding this comment.
4. answer_sdp can survive a successful answer2() and leak into the next answer.
The rollback below only clears call->answer_sdp when pjsua_call_answer2() returns neither PJ_SUCCESS nor PJ_EPENDING, which assumes answer2 always consumes it on success. That holds only while this branch's entry condition matches answer2's media-init condition — and they now differ by the !(call->opt.flag & PJSUA_CALL_MEDIA_APP_MANAGED) test.
Concrete case: opt carries PJSUA_CALL_MEDIA_APP_MANAGED while call->opt does not, and media is already prepared (med_prov_cnt != 0 — e.g. an offerless INVITE with Replaces). Validation's app-managed rejection is skipped on the prepared path, apply_call_setting() then makes the call app-managed, answer2 skips media init, and answer_sdp is left set. The next on_answer_call_med_tp_complete() for this call picks up that stale SDP as its local SDP.
There's a smaller version of the same window between the pjsip_dlg_dec_lock() here and answer2's acquire_call(), if the negotiator state advances in between.
Clearing answer_sdp unconditionally on the success path — or having answer2 signal whether it consumed it — would make this robust rather than coincidental.
|
|
||
| try { | ||
| call->onCallSendAck(prm); | ||
| } catch (Error &err) { |
There was a problem hiding this comment.
5. Catch everything, not just Error.
Thank you for adding this — it closes the realistic case. But an override that throws anything else (std::bad_alloc, std::runtime_error, or a director-translated exception from a SWIG binding) still unwinds through pjsua_call_on_send_ack → inv_send_ack → mod_inv_on_tsx_state, C frames with the dialog lock held.
Since the consequence is std::terminate rather than a handled error, the catch-all is worth having:
} catch (Error &err) {
PJ_LOG(1,(THIS_FILE, "onCallSendAck() error: %s", err.info().c_str()));
return PJ_FALSE;
} catch (...) {
PJ_LOG(1,(THIS_FILE, "onCallSendAck() threw an unknown exception"));
return PJ_FALSE;
}PJ_FALSE keeps the guaranteed fallback ACK, so a throwing override degrades gracefully instead of taking the process down.
| last_res->msg->body = NULL; | ||
| } | ||
|
|
||
| /* An asynchronous confirmed UAS re-INVITE without an SDP offer is |
There was a problem hiding this comment.
6. This changes behaviour for direct pjsip users — worth a release note.
Previously a 2xx answer to an offerless re-INVITE with the negotiator DONE produced a bodyless 200, since process_answer() has no DONE branch. Now it always generates a fresh offer, via on_create_offer() or the pjmedia_sdp_neg_send_local_offer() fallback.
The new behaviour is the RFC-correct one and your new inv_offer_answer_test case asserts it, so no objection. But it's a silent behaviour change for applications sitting directly on pjsip rather than pjsua, and the Release Notes in this PR only mention on_call_rx_reinvite. A line covering pjsip_inv_answer() would save someone a confusing debugging session.
Added support for custom ACK sending callback in PJSUA2 for both synchronous and asynchronous. Added tests for late SDP negotiation. Co-Authored-By: Pavel Spalek <Pavel.SPALEK@frequentis.com>
ua_cfg.cb.on_call_send_ack qurantee that it sends ACK (comment 1,2,3) Default implemetation of void onCallSendAck(OnCallSendAckParam &prm) does not throw exception. (comment 4) The application's rejection code is not ignored (comment 5) Fixed ABI break (comment 6) Reverted condition changes because it is not necessary for our cases(comment 7,8) Fixed guards in pjsua_call_send_ack() (comment 9) Removed dead assignment (comment 11) Instead sending stale sdp offer PJ_EINVALIDOP is returned when on_create_offer() will not return sdp(comment 10). When send 100 in async 200 OK branch fails then transaction is terminated (comment 12).
- Solved re-acquiring the call lock while the dialog lock is held.(comment 1) - When pjsua_call_on_create_offer() leaves *offer NULL than 500 is answered.(comment 2) - New UaConfig.enableCallSendAckCallback configuration option added. When true PJUSA2 will call onSendAckCallback(). Default is false.(comment 3) - Fixed comment for OnCallSendAckParam.suppressSendAck.(comment 4) - Added check if inv->neg is NULL. (comment 5) - call_inv_send_ack() was made static.(comment 6) - Fixed comment for onCallSendAck().(comment 8) - Added cseq into OnCallSendAckParam.(comment 9) - dummy-codecs added into --help and into write_settings().(comment 10)
- guard moved(comment 1) - unitialized response in no longer in code because of rebase on current master(comment 2) - added warning log(comment 3) - unified synchronous and asynchronous response when on_create_offer() doesn't return any sdp. Now it sends OK with current SDP.(comment 4) - fixed documentation comments(comment 5) - SDP offer is not sent if there is non SDP body in re-INVITE(comment 6) - added unit tests for (re-)INVITE with late SDP(comment 7) - fixed wrong naming in sip tests(comment 8) - changed pjsua_call_answer_with_sdp() to work with sdp offer without need to use on_create_offer/on_call_sdp_created
- added validation of initial local SDP offer and check that sdp media count matches call media(comment 1) - onCallSendAck() is guarded with try block(comment 2) - added fallback to acquire_call() when call_id doesn't match(comment 3) - apply_call_setting() is called only when options changed(comment 4) - added missing condition(comment 5) - fixed wording(comment 6)
- Validation of the SDP offer in the response is performed for every (re)INVITE(comment 1) - nonreliable responses with sdp offer are denied with PJ_EINVALIDOP(comment 2) - restored rejection in pjsua_call_send_ack() in case when it is called synchronously from on_call_send_ack() callback but for another call leg(comment 3) - fixed clearing answer_sdp(comment 4) - added catch block for all exceptions(comment 5)
ae8cb4e to
8fd39b2
Compare
sauwming
left a comment
There was a problem hiding this comment.
Round 6 on 8fd39b26b. All 20 comments from rounds 1-5 are addressed — including the two load-bearing ones: the med_prov_cnt validation now runs for every application-supplied local offer, and pjsua_call_send_ack() is back to the PJ_EINVALIDOP rejection with the restriction documented in pjsua.h. Locally: clean CMake build with -Wall, git diff --check clean, and pjsua_late_sdp_offer_test, inv_offer_answer_test and pjsua_call_test all pass.
The important news is from CI, and it's mostly good. The full matrix finally ran on the previous head (836b5a67). Your new tests executed and passed on every platform — pjsua_late_sdp_offer_test [OK] on both Linux and Windows. That was my standing objection since round 2, and it's now genuinely closed.
Two jobs failed, and only one of them is yours:
1. inv_offer_answer_test failed on Windows (OpenSSL / pjsip,pjsua), and nowhere else:
Logs for inv_offer_answer_test [rc:-63]:
Standard INVITE with offer
Offerless re-INVITE with NULL application offer
expected 200 with active SDP (1 media), got 200 with SDP=0 (0 media), callback=1
I think you already fixed this. Round 5 added Call-ID/CSeq matching and the is_user_equal(..., "inv_offer_answer_test") filter to log_on_rx_msg(), and moved expect_null_offer_response = PJ_TRUE to just before the send — which is exactly the cure for an observer latching onto an unrelated 200. sip_inv.c is byte-identical between the two heads, so the production path never changed; this reads as a test race that was more likely to lose on the slower Windows runner. I've approved the workflows on this head so we can confirm rather than assume.
2. Ubuntu (OpenH264+VPX) is not yours. TSan reported a lock-order-inversion entirely inside the presence module — pres_on_rx_request taking a dialog group lock under PJSUA_LOCK in one order and pres_evsub_on_srv_state taking them in the other, during pjsua_pres_test. All 37 tests passed; the job failed only because TSan reported one warning. Pre-existing and unrelated to late SDP. (Master has since added entries to tests/sanitizers/tsan.supp, so this may already be quiet on a rebase.)
So my position on merge-readiness: the code looks right to me and I have no blocking objections to it. What I can't do yet is call it merge-ready, because the only CI evidence we have shows one of its own tests failing, and the fix for that failure has itself never run in CI. One green run on this head and I'd be happy.
The findings below are all minor — 1 and 2 are narrow state-machine gaps worth a look, 3 is a doc gap, 4-6 are cosmetic. None would hold up a merge on its own.
| return PJ_ENOMEM; | ||
| } | ||
| pjsip_dlg_dec_lock(dlg); | ||
| return call_answer_with_pending_sdp(call_id, opt, code, reason, |
There was a problem hiding this comment.
1. This return bypasses the old_neg rollback.
The sibling non-pending path below falls through to the rollback block at the end of pjsua_call_answer_with_sdp(); this one returns directly.
If pjsip_inv_answer() or pjsip_inv_send_msg() fails after on_answer_call_med_tp_complete() has already run pjsip_inv_set_local_sdp(), inv->neg is left in LOCAL_OFFER with nothing to clean it up. A retry then takes the local_offer == PJ_FALSE path and dies in pjsip_inv_set_local_sdp()'s else with PJMEDIA_SDPNEG_EINSTATE — so the call can't be answered at all afterwards.
Either apply the same rollback here, or have call_answer_with_pending_sdp() report whether it installed the offer so the caller can decide.
There was a problem hiding this comment.
Rollback applied to all branches.
| return PJ_EINVALIDOP; | ||
| } | ||
|
|
||
| if (code < 300 && sdp && local_offer) |
There was a problem hiding this comment.
2. An SDP supplied with a final error code falls through unvalidated.
The guard is code < 300, so pjsua_call_answer_with_sdp(id, sdp, NULL, 488, ...) skips this block entirely and reaches pjsip_inv_set_local_sdp() at line 3692.
On a confirmed call (negotiator DONE) that calls pjmedia_sdp_neg_modify_local_offer2() and moves the negotiator to LOCAL_OFFER. Meanwhile pjsip_inv_answer() strips the body for st_code >= 300, and process_answer()'s cancel path only handles REMOTE_OFFER. Net result: rejecting an offerless re-INVITE sends a bodyless 488 and leaves a dangling local offer, after which pjsip_inv_reinvite() returns EINSTATE for the remainder of the call.
Before this PR the same call returned PJMEDIA_SDPNEG_EINSTATE from pjsip_inv_set_sdp_answer() and changed no state, so this is a small regression.
Rejecting sdp outright when code >= 300 seems cleanest — there's no legitimate reason to attach an offer to a final error response.
There was a problem hiding this comment.
When code >= 300 and sdp supplied than it is rejected
| * request. If its wholeSdp is empty, no SDP body | ||
| * will be attached. | ||
| */ | ||
| void sendAck(int cseq, const SdpSession &sdp = SdpSession()) |
There was a problem hiding this comment.
3. Mirror the same-call-id restriction here.
pjsua_call_send_ack() gained the restriction note in pjsua.h (lines 7005-7011) — thank you — but Call::sendAck() doesn't carry it.
A PJSUA2 application doing exactly the 3PCC bridging that the onCallSendAck() docs advertise will call sendAck() for the other leg and get an Error carrying PJ_EINVALIDOP with no explanation. One sentence mirroring the C doc would save them the hunt.
| * along with this program; if not, write to the Free Software | ||
| * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| */ | ||
| #include <pj/types.h> |
There was a problem hiding this comment.
4. These two includes are redundant.
<pj/types.h> and <pjmedia/sdp_neg.h> are both already pulled in transitively by <pjsua-lib/pjsua.h> on the next line, and they sit above the module's own headers, against this file's existing ordering convention.
Cosmetic — worth dropping while you're in here.
- fixed rollback(comment 1) - rejecting SDP when code >= 300(comment 2) - fixed documentation of sendAck()(comment 3) - removed includes(comment 4)
Description
Motivation and Context
This enables applications to complete SDP offer/answer negotiation when the offer or answer must be provided later in the INVITE/re-INVITE exchange, while allowing applications to control ACK handling when needed.
How Has This Been Tested?
Types of changes
Checklist:
Release Notes