`sofia_handle_sip_i_notify()` answered an in-dialog `refer` NOTIFY and then
fell through to the generic handling, which sent a second response and,
on a terminated subscription, destroyed the dialog's handle. A channel kept
up after the REFER outcome (`sip_refer_continue_after_reply=true`) could no
longer send a REFER, re-INVITE or BYE. Return right after the `200 OK`;
`refer` NOTIFYs are no longer forwarded under `proxy-notify-events`.
Co-authored-by: Chris Rienzo <chris@signalwire.com>
The caller profile fields hold the output of `switch_sanitize_number()`,
but the unchanged check compared them against the raw candidate, so a
value the sanitizer alters never matched and the update repeated on every
message. Sanitize once before the comparison, and reuse the result for the
stores and the log.
Cleanups in the same function:
- Stop reading the name and number channel variables up front. Those reads
allocate from the session pool and are discarded whenever a SIP field
supplies the value; the fallbacks below cover the rest. The number's
`sip_to` branch moves there too, keeping the channel variable ahead of
the To user.
- Take the scratch copy the comparison needs from the heap rather than the
pool, so an update that changes nothing costs the session no memory.
- Declare `name` as `const char *` and retarget the display quote strip at
`dup`, which owns the buffer. Removes four casts that handed out a
writable pointer into the parsed message.
`switch_url_encode()` encodes with `double_encode` false, so a `%XX`
already present in `json_text` was emitted unchanged and became
indistinguishable from the escapes the body encoding added itself.
Decoding the body then consumed the values' own escapes as well, and
the document no longer parsed. Values arrive pre-encoded whenever
`encode-values` is on, and can hold percent-hex text regardless of it.
Encode with `switch_url_encode_opt(..., SWITCH_TRUE)` so `%` is encoded
too and the body decodes back to exactly the serialized document.
`mod_xml_cdr` and `mod_format_cdr` already encode their bodies this way.
- Size the escape buffer `* 3 + 1`. `switch_url_encode_opt()` reserves
the terminator from the length it is given, so `* 3` dropped the last
escaped character when every byte needed encoding. The base64 call now
takes that length directly.
- Warn at load when `encode` and `encode-values` are both on, since the
values stay encoded after the body is decoded.
- Correct both encoding config comments; the `encode` one described only
`base64`.
Adds core coverage in `tests/unit/switch_utils.c` for both
`switch_url_encode_opt()` modes, its output bounds, and a JSON body
encoded and decoded once.
* events: avoid O(n^2) header dedup when cloning EF_UNIQ_HEADERS events
switch_event_dup() copies a source event header by header via
switch_event_add_header_string(). When the source carries
EF_UNIQ_HEADERS (SWITCH_EVENT_CHANNEL_DATA / REQUEST_PARAMS / MESSAGE --
e.g. a channel's variable list), the destination inherits the flag, so
switch_event_base_add_header() runs a full switch_event_del_header()
linear scan of the partially-built list on every add to enforce
uniqueness. Cloning an N-header event is therefore O(n^2).
That scan is redundant while cloning: the source already enforced
uniqueness on insert, so copying it cannot introduce duplicates
regardless of the per-add scan. Clear EF_UNIQ_HEADERS on the
destination for the duration of the copy loop, then restore it.
This is a hot path. switch_channel_execute_on() -- invoked on ring,
answer, media, transfer, park, record and playback -- calls
switch_core_get_variables() and switch_channel_get_variables(), each of
which dup()s an EF_UNIQ_HEADERS event. On a production voicemail server
carrying ~191 channel variables per call, switch_event_del_header_val()
accounted for 47-69% of all FreeSWITCH CPU time in on-box perf profiles.
Microbenchmark (tests/unit/switch_event.c, switch_event_dup of an
N-header CHANNEL_DATA event, 4000 iterations, time per dup):
N before after
50 4.80 us 3.19 us
100 11.34 us 5.88 us
191 28.29 us 10.99 us (2.6x)
400 107.05 us 23.27 us (4.6x)
"before" scales ~O(n^2); "after" is linear. Behaviour is unchanged: the
clone has identical headers, values and order, keeps EF_UNIQ_HEADERS,
and still enforces uniqueness on subsequent adds (covered by the test).
Signed-off-by: Calvin Ellison <cellison@youmail.com>
* tests: cover switch_event_dup uniqueness and add a dup benchmark
Adds a dup_uniq_bench case to the switch_event unit test. For an
EF_UNIQ_HEADERS (CHANNEL_DATA) source event it verifies the clone
preserves header count, values and the EF_UNIQ_HEADERS flag, and that
re-setting an existing key does not produce a duplicate in the clone.
It also prints a switch_event_dup timing sweep across header counts,
used to validate the O(n^2) -> O(n) change in switch_event_dup().
Signed-off-by: Calvin Ellison <cellison@youmail.com>
* tests: add dup_faithful_copy regression for switch_event_dup
Covers the EF_UNIQ_HEADERS edge case for the O(n^2)->O(n) dup change: a well-formed EF_UNIQ source stays unique through dup, and a malformed source already holding duplicate names (only possible if headers were added before the flag was set) is copied faithfully rather than silently collapsed to the last value.
Signed-off-by: Calvin Ellison <cellison@youmail.com>
* tests: gate dup_uniq_bench behind #ifdef BENCHMARK
The dup_uniq_bench timing sweep is a benchmark rather than a pass/fail correctness test. Guard the whole test with #ifdef BENCHMARK (matching the existing benchmark in this file) so it stays out of normal test runs; dup_faithful_copy continues to cover correctness of the switch_event_dup change.
Signed-off-by: Calvin Ellison <cellison@youmail.com>
* switch_event: clarify EF_UNIQ_HEADERS dup comment, brace one-line test loops
Address review feedback on the switch_event_dup change:
- src/switch_event.c: the fast-path comment claimed todup's headers are "already unique" unconditionally. That only holds when EF_UNIQ_HEADERS is set (names are deduped on insert); reword to scope the claim to that case and to the verbatim-copy intent.
- tests/unit/switch_event.c: expand one-line loops to braced form per the SignalWire coding guidelines.
Signed-off-by: Calvin Ellison <cellison@youmail.com>
---------
Signed-off-by: Calvin Ellison <cellison@youmail.com>
Co-authored-by: Calvin Ellison <cellison@youmail.com>
A client message can select which chat proto handles it, and the `api` proto
runs the address as a FreeSWITCH API command. That route does not pass through
`verto.fsapi`, so the per-user fsapi permission does not apply to it. The new
per-profile `enable-chat-api-proto` param controls the route and is off unless
set; a message selecting the proto without it is refused and logged.
The proto compare is case-insensitive, matching the chat interface registry,
which is created with `switch_core_hash_init_nocase()`.
Only the `api` proto is gated. A message carrying no proto selector, or one
naming any other chat proto, routes exactly as before.
The param ships commented out in both vanilla verto profiles.
The chat layer lets an inbound SIP MESSAGE select which chat proto handles it,
and the `api` proto runs the address as a FreeSWITCH API command. The new
per-profile `enable-chat-api-proto` param controls that route and is off unless
set. A MESSAGE selecting the proto on a profile without it is answered 403 and
logged with the source and the requested command.
The proto compare is case-insensitive, matching the chat interface registry,
which is created with `switch_core_hash_init_nocase()`.
The param ships commented out in the four vanilla profiles and the mod_sofia
sofia.conf.xml sample.
`sofia_presence_handle_sip_i_message()` splits the chat proto selector off a
truncating copy of the To user. Split only a user that fits the buffer; an
over-long one is handled as a plain address.
`timerfd_gettime()` returns the time remaining until the next periodic tick,
regardless of how many previous ticks have already fired and remain unread.
Replace with `poll()` which correctly checks for pending unread expirations.
`read_rtp_packet()` runs `switch_core_timer_check()` on every empty
read, and that logs an error when `rtp_session->timer` is zeroed, the
normal state for T.38, proxy media, and a disabled timer. Test
`timer.timer_interface` first so those sessions stay quiet; the branch
itself is not reachable without a timer, so packet handling is unchanged.
`switch_silk_decode()` ran its `do`/`while` decode loop while the SDK
kept reporting more internal frames, advancing the output pointer each
pass with no cap. The destination is a fixed-size PCM buffer, so a
stream reporting more internal frames than a conformant packet can
carry let the accumulated write run past its end.
Stop after `MAX_INPUT_FRAMES` internal frames, the most a conformant
SILK packet can hold, so the accumulated PCM stays within the
destination regardless of the bitstream.
Declare the per-pass sample count inside the loop so it resets to zero
each iteration, and advance the output pointer and length only when it
is positive. A pass that writes no samples, including a tolerated FEC
payload error that leaves the count untouched, then contributes nothing
instead of advancing on a stale count from an earlier pass or on a
negative value.
`switch_stun_packet_attribute_get_xor_mapped_address()` read and
XOR-rewrote a 16-byte IPv6 address whenever the `family` byte was 2,
without checking the attribute value was that long. Reject `family == 2`
values shorter than `sizeof(switch_stun_ipv6_t)` and clear the output
address and port. Adds a regression test.
`ws_read_frame()` had two defects in the framing path:
- After header parsing, the remaining payload count
`need = plen - (datalen - header)` could go negative when the
initial read buffered more bytes than the frame's declared
length, even with an in-range `plen`. A negative `need` passed
the signed size guard and reached `ws_raw_read()` as a `size_t`
near its maximum, driving a `memcpy` past `wsh->buffer`. Reject
`need < 0` with a protocol-error close before the read loop.
- The loop filling the frame header called `ws_raw_read()` without
checking its result, so a connection that stopped delivering
header bytes left the loop with no terminating condition,
spinning or hanging the handler thread. Close on a non-advancing
read, matching the payload read loop.
`rtmp_rtmp2rtpH264` read the two leading bytes that select the parse
branch (`data[0]` and `data[1]`) before any length check, so a video
body shorter than 2 bytes read past the buffer. Reject `len < 2` at
entry, before either classifier byte is dereferenced.
In the NAL-unit branch, move the `pdata = data + 5` assignment below
its `len < 5` check so the pointer is never formed past the end of a
short buffer.
Read the 2-byte big-endian SPS and PPS length prefixes byte-wise
(`(pdata[0] << 8) | pdata[1]`) instead of `ntohs(*(uint16_t *)pdata)`.
`pdata` walks a byte buffer at wire-controlled offsets, so the cast was
an unaligned 16-bit load and a strict-aliasing violation; the byte-wise
read is alignment- and endianness-independent and matches the idiom
already used in the NAL-unit branch.
In `rtmp_read_video_frame`, a buffered record is `len + 6` bytes (2-byte
length prefix, 4-byte timestamp, `len`-byte body), but the guard only
required `inuse >= len` before consuming the whole record. Require
`inuse >= len + 6` so the invariant holds locally instead of relying on
the writer emitting each record atomically.
Bounds and termination fixes across the STUN attribute receive path
in `handle_ice` and `switch_stun_lookup`:
- `switch_stun_packet_next_attribute` and its `_hbo` variant now
confirm the 4-byte attribute header is fully within `end` before
dereferencing `type`/`length`, and include the header when checking
that the value fits, so a truncated or overrunning attribute is not
read past the buffer.
- Compute `end_buf` as the 20-byte STUN header plus the attribute
section (`SWITCH_STUN_PACKET_MIN_LEN + header.length`) so the walk
covers every attribute, including trailing ones.
- Make `switch_stun_packet_next_attribute` the sole loop terminator
and drop the redundant `xlen` guard; its seed differed between the
two functions and could skip a trailing attribute in
`switch_stun_lookup`.
- `switch_stun_packet_attribute_get_username` reserves a byte for the
terminator and always NUL-terminates, since callers use the result
as a C string.
switch_b64_encode wrote each base64 character before checking the
output bound, and computed that bound as `bytes >= (int)olen - 1`.
Both are unsafe:
- Writing before the check meant a buffer with no room for a data
byte still received one: `olen == 1` wrote a character at index 0
and the NUL at index 1, and `olen == 0` (where `(int)olen - 1` is
-1) wrote two bytes, both past the end.
- Casting the unsigned `olen` to `int` truncated it above INT_MAX,
producing a wrong bound for very large buffers.
Reject `olen == 0`, make the output index `bytes` a `size_t`, and test
`bytes + 1 >= olen` before each write, so the bound never casts or
underflows and always leaves the last byte for the terminator. Apply
the same `bytes + 1 < olen` guard to the trailing partial-group
character and the `=` padding, so no write can pass the end.
Remove the dead line-wrap counter `y`: it only gated a commented-out
newline emit, so it counted and reset with no effect on the output.
Add unit tests covering the output-bound edges for every write site.
switch_b64_decode bounded its writes with `ol >= olen - 1`, where
`olen` is unsigned. An `olen` of 0 made `olen - 1` wrap to `SIZE_MAX`,
so the bound never fired and the loop wrote the entire decoded input
plus a trailing NUL past the destination. Reject `olen == 0` and test
`ol + 1 >= olen` before each write, so the comparison never subtracts
from an unsigned and always leaves room for the terminator.
The alphabet lookup table `l64` was a `char` indexed by a `char`,
which is unsafe whichever way `char` is signed:
- Where `char` is signed, an input byte >= 0x80 became a negative
index and read before the table.
- Where `char` is unsigned, the `-1` "not in alphabet" sentinel was
stored as 255, so the skip test never matched and non-alphabet
bytes were folded in as data.
Make `l64` a `signed char` and index it with `(unsigned char)`, so the
sentinel survives and the index stays in range on every platform.
Change the bit accumulator `b` from `int` to `unsigned int` to avoid
signed-overflow undefined behavior on long input; decoded output is
unchanged.
Add unit tests: an encode/decode round-trip across the padding cases,
the output-bound edges (`olen` of 0, 1, and a truncating buffer), and
a non-alphabet byte (including one >= 0x80) that must be skipped.
Two invoke handlers read an AMF argument's value without first
checking its type, so a wrong-typed argument reads the wrong union
member:
- `rtmp_i_connect()` passed the first invoke argument straight into
`amf0_object_get()`, which walks the value as a node list. Treat
the command object as a field source only when it is an AMF object
or ECMA array, otherwise leave the lookups empty.
- `rtmp_i_receiveaudio()` and `rtmp_i_receivevideo()` read the flag
argument with `amf0_boolean_get_value()`, which returns the raw
union byte. Use `amf0_get_boolean()`, which yields `SWITCH_FALSE`
unless the argument is an AMF boolean.
In `udptl_rx_packet()`, decoded values from the error-recovery data were
used without checking them against their destination buffers:
- The FEC entry count is read as a single byte and drives both the
decode loop and the stored `fec_entries` later walked by the
reconstruction loop, allowing writes past the `fec[]`/`fec_len[]`
arrays, which hold only `LOCAL_FAX_MAX_FEC_PACKETS` entries. Reject a
larger count. `fec_entries` was also stored before the elements were
decoded, so a mid-loop reject left the slot advertising a count whose
`fec_len[]`/`fec[]` were stale or out of range; since `s->rx[]`
persists across packets, a later reconstruction pass could copy such a
stale length past the fixed `s->rx[].buf`. Commit the count only after
every element is decoded and bound-checked.
- Each redundancy secondary element length was copied into the fixed
`LOCAL_FAX_MAX_DATAGRAM`-sized `s->rx[].buf` without a size check,
unlike the primary packet. Reject an overlength element at decode
time.
`http_directory_auth()` builds the expected "user@domain:password"
token into `z[256]` and base64-encodes it to compare against the
client's Authorization header. Encode with `switch_b64_encode`, which
stops at its output-length argument and always NUL-terminates, and
size `t` from `sizeof(z)`: base64 emits 4 output bytes per 3 input
bytes (final partial group rounded up) plus a NUL, so
`4 * ((sizeof(z) + 2) / 3) + 1` holds the encoding of any `z`. The
encode is confined to `t`, matching the `switch_b64_decode` already
used for the inbound header.
Also in the same function:
- Drop the now-unused `#include <xmlrpc-c/base64_int.h>`, an xmlrpc-c
internal header that only declared the removed encoder.
- Compare 4 bytes, not 3, when stripping a leading `www.` from the
virtual-host `Host:` name. A 3-byte compare also matches hosts like
`www2.example.com` and then strips 4 bytes, yielding `.example.com`
and a failed directory lookup; for a bare `www` it advanced one
byte past the terminating NUL.
* Merge commit from fork
* [core] Fix XML escape encoder overrun and unsigned-char UTF-8 gate
`switch_xml_ampencode()` had two independent defects in its UTF-8
numeric-escape path.
Buffer overrun: the encoder grows its destination once per source byte,
but the realloc margin reserved only the 10 data characters of the
widest escape `"&#x%X;"` (a 21-bit code point rendered as 6 hex digits),
not the terminating NUL that `sprintf` also writes. At the margin
boundary that NUL landed one byte past the allocation. Reserve 11 bytes
in the guard (10 data chars plus the NUL) and emit the escape with
`snprintf` bounded to the remaining space, so the write stays in bounds
even if the margin is ever miscounted. The other escape sinks are all
within the widened margin and are unchanged.
Char signedness: the lead-byte test `(*s >> 8) & 0x01` reads bit 8 of a
plain `char`, which exists only after sign extension. Where `char` is
signed the high bit sign-extends and the test passes; where `char` is
unsigned it is always zero, so the numeric-escape path never ran and
multi-byte UTF-8 was emitted raw, making serialized XML differ by
architecture. Test bit 7 directly with `(*s & 0x80)`, correct regardless
of `char` signedness. This also makes the overrun fix effective on
unsigned-`char` builds, where the escape path now runs.
Add unit test `test_utf_8_wide_codepoint`, which serializes U+10FFFF and
long runs of it across buffer reallocations, sweeping an ASCII prefix so
an escape is emitted at the minimum-headroom offset, and asserts the
full serialized length.
`sofia_dialog_probe_callback()` filled its fixed 512-byte
`remote_display_buf` with unbounded `strcpy`. On the default path the
source is the stored To-header user part, which `sip_dialogs.sip_to_user`
holds verbatim from the wire with no truncation, so a dialog row
addressed to a user part longer than 511 bytes overflowed the buffer
when a `dialog`-package SUBSCRIBE ran the probe.
Copy with `snprintf()` bounded by `sizeof(remote_display_buf)` on every
path that writes it. The four proto branches write short fixed strings
and could not overflow; they are converted for consistency. User parts
under 512 bytes are copied unchanged.
The FU-A (type 28) handler read the 2-byte FU header and copied
`datalen - 2` bytes into `fua_buf` without confirming the frame held
those header bytes. On a 1-byte frame `datalen - 2` wraps to a huge
unsigned length passed to `switch_buffer_write`, and `q[1]` is read
past the payload. Reject FU-A frames shorter than 2 bytes.
Also guard the function entry: bail when `datalen < 1` before reading
`payload[0]` for the NAL type, so a zero-length frame does not read
past the payload.
`get_display_name_from_contact()` copied the SIP Contact display-name
into the caller's fixed 512-byte `remote_display_buf` with an unbounded
`strcpy`, so a stored dialog row with an over-long Contact could
overflow the stack buffer when a `dialog`-package SUBSCRIBE runs the
probe.
Thread the destination size into the helper and copy with
`switch_copy_string()`. Guard the helper against a zero
`dst_size` and a `NULL` or empty input, and always null-terminate the
destination.
In rtmp_rtp2rtmpH264 the STAP-A (type 24) aggregation loop read a
2-byte NALU length prefix at the end of the payload without ensuring
both bytes were in bounds, and copied each NALU without checking its
declared size against the remaining aggregation payload. Stop the loop
one byte earlier so the length prefix is always present, and skip any
NALU whose size exceeds the bytes left.
When building the AVC sequence header, validate the SPS and PPS sizes
before writing: require at least the 4 SPS profile bytes copied into
the header, and confirm the fixed framing plus both payloads fit `buf`
before emitting. Oversized SPS/PPS are logged and skipped instead of
overrunning the stack buffer.
`rtmp_rtmp2rtpH264()` parses two kinds of inbound H.264 RTMP video messages: an
AVC configuration record (`0x17/0x00`) that captures SPS/PPS, and NAL-unit
messages (`0x17`/`0x27` with `0x01`). Both paths trusted wire-supplied sizes and
counts without checking them against the bytes actually present, leading to
out-of-bounds reads.
NAL-unit walk:
- The length-prefix walk advanced the cursor and decremented the unsigned
remaining-byte counter by the wire NAL size with no check that the size fit.
A NAL size larger than the remaining payload underflowed the counter to near
`UINT32_MAX`, kept the loop running, and read the next size prefix from a
cursor already past the end of the buffer. The initializer
`pdata_len = len - 5` underflowed the same way for a message shorter than the
5-byte AVC header.
- Reject `len < 5`, change the loop guard to `pdata_len > lenSize` so each
size-prefix read stays in bounds, and reject any NAL whose declared size
exceeds the bytes remaining after its prefix.
AVC configuration record:
- The fixed header fields (`configurationVersion`, `lengthSizeMinusOne`,
`numOfSequenceParameterSets`) plus each 2-byte SPS/PPS length prefix and the
PPS count byte were read with no minimum-length check. The existing per-entry
checks bounded only the SPS/PPS body copies and ran after the length reads.
- Reject `len < 11` before the fixed header, and add a remaining-bytes check
before each `ntohs` length read and before the PPS count byte.
Both changes are correctness-only: well-formed records and NAL streams hit none
of the new guards. Malformed or truncated input is rejected with the existing
"corrupted data" diagnostic.
`rtmp_handle_control()` formats the control-message body into a fixed
200-byte stack buffer with an unbounded `sprintf` loop whose iteration
count is the wire message length. A body of ~70 bytes or more runs the
write off the end of `buf`, corrupting the stack frame; the length is
taken straight from the chunk header and reaches this path before any
login, so a remote peer can trigger it.
Bound the loop with `snprintf` against the remaining space and stop when
the buffer is full. This also caps the iteration count, so the loop can
no longer read `state->buf` past what was reassembled. The hex dump is
debug-only output, so capping it changes nothing operational.
In `msrp_parse_buffer()`, the `range_star` arm of `MSRP_ST_WAIT_BODY`
computed the body length by subtracting the delimiter length and trailing
framing from the received segment length. `payload_bytes` is a
`switch_size_t`, so a short segment wrapped it to near `SIZE_MAX`, which
`switch_msrp_msg_set_payload()` uses to size its allocation and as the
`memcpy` length. This affects both `len - dlen - 5`, whose scan pointer
also addressed memory before `buf`, and `delim_pos - buf - 2`, covered
only by a `switch_assert()` on received data.
Require room for the trailing end-line and the CRLF closing the body
before either is computed; a short segment is incomplete, so the parser
waits for more bytes.
`switch_core_media_add_crypto()` parses the SDP `a=crypto` keysalt and
decodes it into a fixed-size key buffer. Enforce the buffer contract at
the call site:
- Reject a zero-length, negative, or over-long keysalt (the length
check now has both a lower bound and an upper bound against the copy
buffer).
- Copy the keysalt token into a NUL-terminated buffer and decode from
that, so `switch_b64_decode` stops at the token instead of reading on
into the following key material (it consumes input to the NUL and
skips non-base64 bytes).
- Pass the destination buffer size as the decode bound rather than the
parsed token length.
- Require the decoded length to cover the crypto suite's key+salt so the
subsequent copy cannot read past the decoded bytes.
Add `test_add_crypto_keysalt_bounds`, a table-driven test covering the
accepted and rejected keysalt shapes across suites (AES-128/192/256),
including RFC 4568 lifetime/MKI and multi-key lines.
Andrey Volk