Skip to content
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.

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

Fixed account's route set update when modifying account #3825

Merged
merged 4 commits into from
Jan 16, 2024
Merged

Conversation

sauwming
Copy link
Member

@sauwming sauwming commented Jan 10, 2024

There's an issue with account modification via pjsua_acc_modify() when updating account's route_set.

Account's route set is a list that consists of: global proxies + account proxies + service route learnt from update_service_route().
But when modifying account, the learnt service route is never taken into account, causing incorrect list size calculation, and hence incorrect element removal.

The fixes in this PR:

  • Learnt route headers is only removed upon registrar change. Account proxy is only modified upon change.
  • Remove global outbound proxy modification.
    The current implementation seems to be buggy and not working as intended for the following reasons:
    • there's no API to modify the global proxy,
    • even if app modifies pjsua_var.ua_cfg.outbound_proxy internally (such as by including pjsua_internal.h), the route set is built from the list pjsua_var.outbound_proxy, which actually has different content and is never updated.
  • Add new list APIs pj_list_insert_list_before/after().

Thank you to @andreas-wehrmann for the report and fix recommendation.

Copy link
Member

@nanangizz nanangizz left a comment

Choose a reason for hiding this comment

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

This change seems to drop the existing memory optimization, e.g: currently (shallow) cloning global proxy will only occur if global proxy is changed & (deep) cloning account proxy only occur if account proxy is changed, now both clonings will always occur when whichever proxy setting is changed or reg URI is changed. If some app for some reason modifies registrar URI regularly, this may cause unnecessary/avoidable "memory leak" (as account pool is never reset?).

If the memory optimization can be maintained, it will be ideal, but the new behavior may also be acceptable considering the memory leak scenario seems to be a rare scenario.

@sauwming
Copy link
Member Author

Changes in the latest commit:

  • Learnt route headers is only removed upon registrar change. Account proxy is only modified upon change.
  • Remove global outbound proxy modification.
    The current implementation seems to be buggy and not working as intended for the following reasons:
    • there's no API to modify the global proxy,
    • even if app modifies pjsua_var.ua_cfg.outbound_proxy internally (such as by including pjsua_internal.h), the route set is built from the list pjsua_var.outbound_proxy, which actually has different content and is never updated.
  • Add new list APIs pj_list_insert_list_before/after(). The current APIs pj_list_insert_nodes_before/after() seem to be useless since our list typically has an empty element as its head.

pjlib/include/pj/list_i.h Show resolved Hide resolved
pjlib/include/pj/list.h Outdated Show resolved Hide resolved
pjsip/src/pjsua-lib/pjsua_acc.c Outdated Show resolved Hide resolved
pjlib/include/pj/list.h Outdated Show resolved Hide resolved
@sauwming sauwming merged commit 98d51a0 into master Jan 16, 2024
35 checks passed
@sauwming sauwming deleted the acc-route-set branch January 16, 2024 02:12
dshamaev-intermedia pushed a commit to intermedia-net/pjproject that referenced this pull request Apr 5, 2024
dshamaev-intermedia added a commit to intermedia-net/pjproject that referenced this pull request Apr 5, 2024
* Add option to shutdown all transports on IP change (pjsip#3781)

* pjsua_handle_ip_change: Added missing null check for on_ip_changed_progress callback (pjsip#3830)

* Reset stored remote name in dialog (dlg->initial_dest) if transport is server. (pjsip#3783)

* Prevent immediate tsx termination upon transport error (pjsip#3805)

* Fixed issues when adding new media and deinitializing media (pjsip#3821)

* Potential issues when IPv6 is disabled (pjsip#3835)

* Improve IP address change IPv4 <-> IPv6 (pjsip#3910)

* Add some missing unlocks (pjsip#3893)

* add missing unlock (pjsip#3885)

* Retransmit 2xx response when transport is closed (pjsip#3828)

* Fixed account's route set update when modifying account (pjsip#3825)

---------

Co-authored-by: Nanang Izzuddin <[email protected]>
Co-authored-by: sauwming <[email protected]>
Co-authored-by: Santiago De la Cruz <[email protected]>
Co-authored-by: Andreas Wehrmann <[email protected]>
Co-authored-by: Riza Sulistyo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants