nathelper: fix Contact concatenation when fix_nated_contact() runs after topology_hiding()#3824
Open
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Open
Conversation
…ter topology_hiding() (GH OpenSIPS#2172) When topology_hiding() is called before fix_nated_contact() and the Contact header lacks angle-bracket enclosure (<>), both functions create LUMP_DEL entries at the same buffer offset. The message builder's last_del exception prevents the second lump from being skipped, causing both replacement values to be concatenated into a single malformed Contact header. With angle brackets the offsets differ by 1 byte (body.s vs uri.s) so the message builder correctly suppresses the second lump. Without angle brackets body.s == uri.s, producing identical offsets and triggering the bug. Add a helper function contact_already_rewritten() that scans the lump list for a prior LUMP_DEL covering the Contact URI. The new "nated_contact_override" module parameter controls behavior: 0 (default) - skip: let the prior rewrite stand 1 - override: neutralize the prior lump and let fix_nated_contact create its own replacement Both modes prevent the concatenation bug. The check is per-Contact using continue, so multi-Contact messages still process uncovered contacts normally. The lump scan exits early since the list is sorted by offset. Closes: OpenSIPS#2172
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fix_nated_contact()called aftertopology_hiding()produces a malformed Contact header containing two concatenated SIP URIs when the Contact lacks angle-bracket enclosure (<>).Details
Both
topology_hiding()andfix_nated_contact()rewrite the Contact header using the lump mechanism. Each creates aLUMP_DELentry (marking a region for deletion) with anafterchainLUMP_ADDentry (inserting replacement text).topology_hiding()viadelete_existing_contact()deletesmsg->contact->body(the full Contact body).fix_nated_contact_f()deletes fromc->uri.sthrough the host:port portion.The Contact parser (
parser/contact/contact.c:217-222) strips angle brackets fromcontact_t->uri:<>:body.spoints to<(offset N),uri.spoints tosip:(offset N+1) — offsets differ by 1<>:body.sanduri.spoint to the same address — offsets are identicalIn
msg_translator.c,process_lumps()iterates lumps sorted by offset. It has alast_delexception that allows processing of multiple lumps anchored at the same deletion point:When offsets differ by 1 (with brackets), the second lump falls inside the first's deleted region and is correctly skipped. When offsets are identical (no brackets), the
last_delexception prevents the skip, and both replacement values are emitted — producing:The reverse call order (
fix_nated_contact()thentopology_hiding()) is already handled:delete_existing_contact()intopo_hiding_logic.c:319-336scans the lump list for existing Contact DEL lumps and neutralizes them. No equivalent guard exists infix_nated_contact_f().Solution
Add a helper function
contact_already_rewritten()that scansmsg->add_rmfor a priorLUMP_DELof typeHDR_CONTACT_Twhose offset and length fully encompass the current Contact URI. This detects when another function (such astopology_hiding()) has already scheduled the Contact for rewriting.A new module parameter
nated_contact_overridecontrols the behavior when a conflict is detected:0(default): skip this Contact — the prior rewrite stands.fix_nated_contact()returns without creating lumps for this Contact.1: override — neutralize the prior lump by settingop = LUMP_NOPand insertingCOND_FALSEguards on itsbefore/afterchains (the same technique used bydelete_existing_contact()intopo_hiding_logic.c:331-334).fix_nated_contact()then proceeds to create its own replacement.Design notes:
continue(notreturn) inside theget_contact_uri()loop, so only conflicting Contacts are affected; uncovered Contacts in sibling headers are processed normally.del_lump()(data_lump.c:369-377) for early exit: once we see a DEL lump with offset past the URI start, no later lump can fully cover the URI.msg->add_rmis per-processpkgmemory owned by a single worker, processed sequentially in the routing script. This is the same concurrency model used bydelete_existing_contact().crt->u.offset <= uri_off && crt->u.offset + crt->len >= uri_end) intentionally requires full containment. Partial-overlap lumps (e.g., from a priorfix_nated_contact()call) are not matched — that case is already handled by the existing "URI outside buffer" check atnathelper.c:703.Compatibility
nated_contact_override=0preserves the current effective behavior: whentopology_hiding()runs first, its Contact wins. The only change is that the concatenation bug is eliminated.msg_translator.c,data_lump.c). All changes are localized to the nathelper module.HDR_CONTACT_TLUMP_DELfully covering the URI will be detected, not only those fromtopology_hiding(). This is intentional and consistent withdelete_existing_contact()'s general approach.nathelper_admin.xml.Closing issues
closes #2172