Skip to content

refactor(pjsua): commit the new Contact in update_regc_contact() #5311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nanangizz merged 1 commit into from
Oct 8, 2026
Merged

refactor(pjsua): commit the new Contact in update_regc_contact() #5311

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
229 changes: 109 additions & 120 deletions pjsip/src/pjsua-lib/pjsua_acc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2147,17 +2147,23 @@ PJ_DEF(pj_status_t) pjsua_acc_set_online_status2( pjsua_acc_id acc_id,
return PJ_SUCCESS;
}

/* Create reg_contact, adding SIP outbound params and other REGISTER specific
* Contact params, i.e: reg_contact_params, reg_contact_uri_params.
/* Create reg_contact from new_contact (NULL: acc->contact), adding SIP
* outbound params and other REGISTER specific Contact params, i.e:
* reg_contact_params, reg_contact_uri_params.
* With push, also load it into the regc; pass PJ_FALSE where the regc takes
* it from pjsip_regc_init() or regc_tsx_cb()'s param->contact instead.
* Returns PJ_FALSE if the regc refused it, leaving reg_contact as it was.
* Returns PJ_FALSE if the push was refused, leaving the account as it was.
* Otherwise new_contact becomes acc->contact; without push, before the regc
* has seen it.
*/
static pj_bool_t update_regc_contact(pjsua_acc *acc, pj_bool_t push)
static pj_bool_t update_regc_contact(pjsua_acc *acc,
const pj_str_t *new_contact,
pj_bool_t push)
{
pjsua_acc_config *acc_cfg = &acc->cfg;
const pj_str_t *contact = new_contact ? new_contact : &acc->contact;
pj_bool_t need_outbound = PJ_FALSE;
pj_str_t prev_contact = acc->reg_contact;
pj_str_t prev_reg_contact = acc->reg_contact;
unsigned prev_status = acc->rfc5626_status;
const pj_str_t tcp_param = pj_str(";transport=tcp");
const pj_str_t tls_param = pj_str(";transport=tls");
Expand All @@ -2173,8 +2179,8 @@ static pj_bool_t update_regc_contact(pjsua_acc *acc, pj_bool_t push)
if (acc->rfc5626_status == OUTBOUND_NA)
goto done;

if (pj_stristr(&acc->contact, &tcp_param)==NULL &&
pj_stristr(&acc->contact, &tls_param)==NULL)
if (pj_stristr(contact, &tcp_param)==NULL &&
pj_stristr(contact, &tls_param)==NULL)
{
/* Currently we can only do SIP outbound for TCP
* and TLS.
Expand All @@ -2190,134 +2196,125 @@ static pj_bool_t update_regc_contact(pjsua_acc *acc, pj_bool_t push)
pj_ssize_t len;
pj_str_t reg_contact;

acc->rfc5626_status = OUTBOUND_WANTED;
len = acc->contact.slen +
len = contact->slen +
acc->cfg.contact_params.slen +
acc->cfg.reg_contact_params.slen +
acc->cfg.reg_contact_uri_params.slen +
(need_outbound?
(acc->rfc5626_instprm.slen + acc->rfc5626_regprm.slen): 0) +
5; /* allowance */
if (len > acc->contact.slen) {
reg_contact.ptr = (char*) pj_pool_alloc(acc->pool, len);
reg_contact.ptr = (char*) pj_pool_alloc(acc->pool, len);

/* Must be NULL terminated: the buffer is parsed below and by
* pjsip_regc, and the parser requires a NULL terminated input.
/* Must be NULL terminated: the buffer is parsed below and by
* pjsip_regc, and the parser requires a NULL terminated input.
*/
pj_strncpy_with_null(&reg_contact, contact, len);

/* Contact URI params */
if (acc->cfg.reg_contact_uri_params.slen) {
pj_pool_t *pool;
pjsip_contact_hdr *contact_hdr;
/* Only ever handed to pjsip_uri_print(), which dispatches
* on the URI's own vptr, so this must not be narrowed to
* pjsip_sip_uri*: a Contact may legitimately hold another
* scheme and no SIP specific field is read here.
*/
pj_strncpy_with_null(&reg_contact, &acc->contact, len);

/* Contact URI params */
if (acc->cfg.reg_contact_uri_params.slen) {
pj_pool_t *pool;
pjsip_contact_hdr *contact_hdr;
/* Only ever handed to pjsip_uri_print(), which dispatches
* on the URI's own vptr, so this must not be narrowed to
* pjsip_sip_uri*: a Contact may legitimately hold another
* scheme and no SIP specific field is read here.
*/
pjsip_uri *uri = NULL;
pj_str_t uri_param = acc->cfg.reg_contact_uri_params;
const pj_str_t STR_CONTACT = { "Contact", 7 };
char tmp_uri[PJSIP_MAX_URL_SIZE];
pj_ssize_t tmp_len = -1;

/* Get the URI string */
pool = pjsua_pool_create("tmp", 512, 512);
contact_hdr = (pjsip_contact_hdr*)
pjsip_parse_hdr(pool, &STR_CONTACT,
reg_contact.ptr,
reg_contact.slen, NULL);
if (contact_hdr && contact_hdr->uri)
uri = (pjsip_uri*)pjsip_uri_get_uri(contact_hdr->uri);
if (uri) {
tmp_len = pjsip_uri_print(PJSIP_URI_IN_CONTACT_HDR,
uri, tmp_uri,
sizeof(tmp_uri));
}
pj_pool_release(pool);

if (tmp_len > 0) {
pj_ssize_t new_len;

/* Regenerate Contact */
new_len = pj_ansi_snprintf(
reg_contact.ptr, len,
"<%.*s%.*s>%.*s",
(int)tmp_len, tmp_uri,
(int)uri_param.slen, uri_param.ptr,
(int)acc->cfg.contact_params.slen,
acc->cfg.contact_params.ptr);
if (new_len > 0 && new_len < len)
reg_contact.slen = new_len;
else
tmp_len = -1;
}

if (tmp_len <= 0) {
/* Leave reg_contact as the plain copy of acc->contact */
PJ_LOG(1,(THIS_FILE,
"Unable to apply registration Contact URI "
"params of acc %d to Contact '%.*s'",
acc->index, (int)acc->contact.slen,
acc->contact.ptr));
pj_strncpy_with_null(&reg_contact, &acc->contact, len);
}
pjsip_uri *uri = NULL;
pj_str_t uri_param = acc->cfg.reg_contact_uri_params;
const pj_str_t STR_CONTACT = { "Contact", 7 };
char tmp_uri[PJSIP_MAX_URL_SIZE];
pj_ssize_t tmp_len = -1;

/* Get the URI string */
pool = pjsua_pool_create("tmp", 512, 512);
contact_hdr = (pjsip_contact_hdr*)
pjsip_parse_hdr(pool, &STR_CONTACT,
reg_contact.ptr,
reg_contact.slen, NULL);
if (contact_hdr && contact_hdr->uri)
uri = (pjsip_uri*)pjsip_uri_get_uri(contact_hdr->uri);
if (uri) {
tmp_len = pjsip_uri_print(PJSIP_URI_IN_CONTACT_HDR,
uri, tmp_uri,
sizeof(tmp_uri));
}
pj_pool_release(pool);

/* Outbound */
if (need_outbound) {
acc->rfc5626_status = OUTBOUND_WANTED;
if (tmp_len > 0) {
pj_ssize_t new_len;

/* Regenerate Contact */
new_len = pj_ansi_snprintf(
reg_contact.ptr, len,
"<%.*s%.*s>%.*s",
(int)tmp_len, tmp_uri,
(int)uri_param.slen, uri_param.ptr,
(int)acc->cfg.contact_params.slen,
acc->cfg.contact_params.ptr);
if (new_len > 0 && new_len < len)
reg_contact.slen = new_len;
else
tmp_len = -1;
}

/* Need to use outbound, append the contact with
* +sip.instance and reg-id parameters.
*/
pj_strcat(&reg_contact, &acc->rfc5626_regprm);
pj_strcat(&reg_contact, &acc->rfc5626_instprm);
} else {
acc->rfc5626_status = OUTBOUND_NA;
if (tmp_len <= 0) {
/* Leave reg_contact as the plain copy of the Contact */
PJ_LOG(1,(THIS_FILE,
"Unable to apply registration Contact URI "
"params of acc %d to Contact '%.*s'",
acc->index, (int)contact->slen,
contact->ptr));
pj_strncpy_with_null(&reg_contact, contact, len);
}
}

/* Contact params */
pj_strcat(&reg_contact, &acc->cfg.reg_contact_params);
/* Outbound */
if (need_outbound) {
acc->rfc5626_status = OUTBOUND_WANTED;

/* pj_strcat() does not NULL terminate, and the buffer is later
* parsed by pjsip_regc. The 'len' allowance guarantees room.
/* Need to use outbound, append the contact with
* +sip.instance and reg-id parameters.
*/
pj_assert(reg_contact.slen < len);
if (reg_contact.slen < len)
reg_contact.ptr[reg_contact.slen] = '\0';
pj_strcat(&reg_contact, &acc->rfc5626_regprm);
pj_strcat(&reg_contact, &acc->rfc5626_instprm);
} else {
acc->rfc5626_status = OUTBOUND_NA;
}

acc->reg_contact = reg_contact;
/* Contact params */
pj_strcat(&reg_contact, &acc->cfg.reg_contact_params);

PJ_LOG(4,(THIS_FILE,
"Contact for acc %d updated: %.*s",
acc->index,
(int)acc->reg_contact.slen,
acc->reg_contact.ptr));
/* pj_strcat() does not NULL terminate, and the buffer is later
* parsed by pjsip_regc. The 'len' allowance guarantees room.
*/
pj_assert(reg_contact.slen < len);
if (reg_contact.slen < len)
reg_contact.ptr[reg_contact.slen] = '\0';

} else {
/* Outbound is not needed/wanted for the account and there's
* no custom registration Contact params. acc->reg_contact
* is set to the same as acc->contact.
*/
acc->reg_contact = acc->contact;
acc->rfc5626_status = OUTBOUND_NA;
}
acc->reg_contact = reg_contact;
}

if (push && acc->regc) {
pj_status_t status;

status = pjsip_regc_update_contact(acc->regc, 1, &acc->reg_contact);
if (status != PJ_SUCCESS) {
/* set_contact() validates first: the regc kept prev_contact */
/* set_contact() validates first: the regc kept prev_reg_contact */
pjsua_perror(THIS_FILE, "Failed updating registration Contact",
status);
acc->reg_contact = prev_contact;
acc->reg_contact = prev_reg_contact;
acc->rfc5626_status = prev_status;
return PJ_FALSE;
}
}
if (new_contact && new_contact != &acc->contact)
pj_strdup_with_null(acc->pool, &acc->contact, new_contact);

@sauwming sauwming Oct 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: if a future caller passes &acc->contact as new_contact (the doc's "NULL: acc->contact" sort of invites it), pj_strdup_with_null() copies onto itself: dst->ptr is set to a fresh buffer before copying from src->ptr, which is now that same buffer, so the Contact becomes uninitialised bytes. No current caller does this. Maybe pj_assert(new_contact != &acc->contact), or treat it like NULL?

@laconicman laconicman Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treated like NULL now. An assert would compile out in release builds and still copy garbage.


PJ_LOG(4,(THIS_FILE,
"Contact for acc %d updated: %.*s",
acc->index,
(int)acc->reg_contact.slen,
acc->reg_contact.ptr));
return PJ_TRUE;
}

Expand Down Expand Up @@ -2577,7 +2574,7 @@ static pj_bool_t acc_check_nat_addr(pjsua_acc *acc,
pj_bool_t secure;
pjsip_contact_hdr *new_hdr;
pj_in6_addr v6_addr;
pj_str_t prev_contact;
pj_str_t new_contact;

secure = pjsip_transport_get_flag_from_type(tp->key.type) &
PJSIP_TRANSPORT_SECURE;
Expand Down Expand Up @@ -2663,13 +2660,10 @@ static pj_bool_t acc_check_nat_addr(pjsua_acc *acc,
destroy_regc(acc, PJ_TRUE);
}

prev_contact = acc->contact;
pj_strdup2_with_null(acc->pool, &acc->contact, tmp);

if (!update_regc_contact(acc, contact_rewrite_method ==
PJSUA_CONTACT_REWRITE_NO_UNREG))
if (!update_regc_contact(acc, pj_cstr(&new_contact, tmp),
contact_rewrite_method ==
PJSUA_CONTACT_REWRITE_NO_UNREG))
{
acc->contact = prev_contact;
pj_pool_release(pool);
return PJ_FALSE;
}
Expand Down Expand Up @@ -3024,7 +3018,7 @@ static void update_rfc5626_status(pjsua_acc *acc, pjsip_rx_data *rdata)
on_return:
/* Unconfirmed outbound is not in use (RFC 5626 section 6): drop reg-id */
if (was_outbound && acc->rfc5626_status == OUTBOUND_NA)
update_regc_contact(acc, PJ_TRUE);
update_regc_contact(acc, NULL, PJ_TRUE);
PJ_LOG(4,(THIS_FILE, "SIP outbound status for acc %d is %s",
acc->index, (acc->rfc5626_status==OUTBOUND_ACTIVE?
"active": "not active")));
Expand Down Expand Up @@ -3494,8 +3488,7 @@ static pj_status_t pjsua_regc_init(int acc_id)
goto on_return;
}

pj_strdup_with_null(acc->pool, &acc->contact, &tmp_contact);
update_regc_contact(acc, PJ_FALSE);
update_regc_contact(acc, &tmp_contact, PJ_FALSE);
}

status = pjsip_regc_init( acc->regc,
Expand Down Expand Up @@ -5435,12 +5428,8 @@ static void auto_rereg_timer_cb(pj_timer_heap_t *th, pj_timer_entry *te)
goto on_return;
}
} else {
/* A new buffer, so the old Contact survives a refusal */
pj_str_t prev_contact = acc->contact;

pj_strdup_with_null(acc->pool, &acc->contact, &tmp_contact);
if (!update_regc_contact(acc, PJ_TRUE))
acc->contact = prev_contact;
/* On refusal, re-register with the Contact the regc kept */
update_regc_contact(acc, &tmp_contact, PJ_TRUE);

@sauwming sauwming Oct 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value is ignored here, so if the regc refuses the new Contact we go on to re-register with the old one and don't retry. Same at update_rfc5626_status() (line 3023). The old code behaved the same way, so this isn't a regression. Now that the helper reports refusal explicitly, is ignoring it intended here, or should this site e.g. schedule_reregistration() like the reinit failure branch above?

@laconicman laconicman Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended; there is now a one-line comment at the call. set_contact() refuses only a Contact that does not parse (PJSIP_EINVALIDURI), so the result depends on the input alone: rescheduling would just repeat the refusal and never send a REGISTER, while carrying on re-registers with the Contact the regc kept (it validates before touching its list), under the same retry rules as any other REGISTER. In update_rfc5626_status() the candidate is the already accepted Contact minus reg-id and +sip.instance, so a refusal is not expected there.

}
}
pj_pool_release(pool);
Expand Down
Loading