sdp_ops: fix body-lump offsets once $sdp owns the body (SIGABRT/SIGSEGV) - #4237
Open
Lt-Flash wants to merge 2 commits into
Open
sdp_ops: fix body-lump offsets once $sdp owns the body (SIGABRT/SIGSEGV)#4237Lt-Flash wants to merge 2 commits into
Lt-Flash wants to merge 2 commits into
Conversation
added 2 commits
September 3, 2026 20:12
Once $sdp / $sdp.line / $sdp.stream / $sdp.session has touched a message,
get_body() returns a pointer into sdp_ops' own pkg buffer (ops->sdp or
ops->rebuilt_sdp), not into msg->buf. Every body editor that then computes
"body.s - msg->buf" to anchor a lump feeds garbage pointer arithmetic to
del_lump()/insert_new_lump(): del_lump's bounds check aborts the worker
(SIGABRT, "offset exceeds message size"), and apply_msg_changes() in
msg_translator.c - a branch written FOR sdp_ops - does the same
subtraction and then memcpy()s with a size of a few GB (SIGSEGV in
build_req_buf_from_sip_req). The second one needs no module at all:
$sdp = $sdp;
t_relay();
crashes a stock build. The first one fires with rtp_relay_engage(),
rtpengine_offer()/_answer() (sync and async reply paths), mediaproxy,
textops' search_append_body() and sipmsgops' codec editing.
Fix: sdp_ops grows a small API extracted from pv_set_sdp() -
sdp_ops_set_body(), sdp_ops_set_null_body() and sdp_ops_splice_body(),
the latter taking offsets relative to the CURRENT body buffer - and every
affected call site goes through it when have_sdp_ops(msg) is true, leaving
the lump path untouched otherwise. apply_msg_changes() recovers the
received body position the same way calculate_body_diff() already does
(NULL msg->sdp_ops around get_body()), so the two stay in agreement -
one sizes the buffer, the other fills it. codecs.c, whose incremental
multi-lump editing has no sdp_ops equivalent, now fails cleanly with an
error instead of computing an invalid offset.
modules/sngtc carries the same "body.s - msg->buf" pattern but is not
changed here - it cannot be built without the Sangoma library.
Fixes OpenSIPS#4135
…scape pv_parse_sdp_line_name() (which also serves $sdp.session) and pv_parse_sdp_stream_name() lost the final segment of the name whenever it ends with an escape, e.g. the manual's own "$sdp.line(a=rtpmap:101/telephone-event\/)": unescaping shortens in.len and rewinds i, so on the last character the loop exits on the shortened length and the in-loop "(i+1) == in.len" assignment of the last segment never runs. The token prefix was silently dropped and the name degraded into a whole-line match - on reads, and on writes, which then replaced the entire line with the value. Assign whatever trails the last separator after the loop instead, guarded by a flag that the "[" with no following "/" break path clears. Related to OpenSIPS#4135
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
1 participant
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.
Fixes #4135.
Two commits: the crash fix first, then a small parser fix found on the same path.
1. Body-lump offsets once
$sdpowns the bodyOnce
$sdp/$sdp.line/$sdp.stream/$sdp.sessionhas touched a message,get_body()returns a pointer into sdp_ops' own pkg buffer rather than intomsg->buf. Every body editor that then anchors a lump withbody.s - msg->bufis doing pointer arithmetic between two unrelated allocations:del_lump()'s bounds check aborts the worker — the reporter'sSIGABRT,del_lump: offset exceeds message size (-2036890104 > 1068)— viartp_relay_engage(),rtpengine_offer()/rtpengine_answer()(sync and async paths), mediaproxy, textopssearch_append_body(), sipmsgops codec editing.apply_msg_changes()inmsg_translator.c— a branch written for sdp_ops — makes the same subtraction and thenmemcpy()s a few GB:SIGSEGVinbuild_req_buf_from_sip_req(the reporter's second backtrace). This one needs no module at all:crashes a stock build.
Fix. sdp_ops gets a small API extracted from
pv_set_sdp()—sdp_ops_set_body(),sdp_ops_set_null_body(),sdp_ops_splice_body()(offsets relative to the current body buffer) — and each affected call site goes through it whenhave_sdp_ops(msg)is true; the lump path is untouched otherwise.apply_msg_changes()recovers the received body position the waycalculate_body_diff()already does, so the function that sizes the buffer and the one that fills it agree.codecs.cfails cleanly with an error instead of computing an invalid offset — its incremental multi-lump editing has no sdp_ops equivalent.2.
$sdp.*names ending with an escapepv_parse_sdp_line_name()/pv_parse_sdp_stream_name()dropped the last name segment when the name ends with an escape (the manual's owna=rtpmap:101/telephone-event\/), silently turning a token match into a whole-line match — on reads and writes.Verification
A local reproduction rig: the reporter's script shape (
$sdp.linereads +rtpengine_offer()) and the module-free$sdp = $sdp; t_relay()both crash on the unpatched build and relay a well-formed message with the fix (a downstream sink validatesContent-Lengthagainst the body). Builds clean with-Wredundant-decls -Wold-style-definition -Wmissing-field-initializers -Werroron current master.Not in this PR
modules/sngtchas the same pattern at four sites but cannot be built without the Sangoma library, so it is left as is and named here. The token-write side of the same report ($sdp.line(o=/[5]) = "..."being a silent no-op) is a separate change and will follow as its own PR.