Conversation
If __bencode_piece_new() fails (pkg memory exhausted), the function returns -1 after only having assigned buf->pieces, leaving free_list and error with their previous (stack garbage) values. Callers, which cannot distinguish the failure stage, routinely run bencode_buffer_free() on the returned error - and the free routine starts by dereferencing buf->free_list, producing undefined behaviour (reading a garbage pointer and calling through it). Zero the whole struct up front: on the failure path the buffer is now an empty, safely freeable object (both loops in bencode_buffer_free() simply do not run), while on the success path all three fields are explicitly assigned, so behaviour is unchanged.
RTPE_IO_ERROR_CLOSE() expands to close(_fd) followed by (_fd) = -1,
but the resume handler passed param->node->idx - the node's array
index - instead of the failed fd. Two consequences on the
EPIPE/EBADF branch of the macro:
* close(idx) closes whichever descriptor happens to have that
number; for the first nodes in a set this is one of the stdio
fds (idx 0, 1 or 2), silently breaking logging/stdin;
* node->idx is overwritten with -1, so every later
rtpe_socks[node->idx] access indexes the array out of bounds.
Pass the fd that actually failed; it is a per-command socket created
by start_async_send_rtpe_command(), not the shared rtpe_socks slot,
so no array slot needs to be invalidated here.
An async rtpengine command builds a fresh UDP socket per command. connect() on it makes the kernel autobind a local source port from ip_local_port_range first, so once that pool (shared between TCP and UDP) runs out, connect() fails with EAGAIN, and socket() may fail with EMFILE/ENFILE/ENOMEM. All of these are local machine conditions and say nothing about the health of the rtpengine node, yet the code treated them like any other send failure: it jumped to the badproxy label and disabled the node for rtpengine_disable_tout. With the pool exhausted, every command towards every node fails at once, so a single local resource shortage ends up flagging ALL nodes as down - while the long-lived rtpe_socks[] probes (already bound, no new port needed) keep succeeding, making the nodes flip between enabled and disabled until the process is restarted. Route the local resource errnos (EAGAIN, EADDRNOTAVAIL, ENOBUFS, ENOMEM, EMFILE, ENFILE) to the existing error label instead, which only releases the fd without touching rn_disabled. Errors that do reflect node/network reachability (ECONNREFUSED, ENETUNREACH, EPIPE ...) still disable the node as before. The check is only performed immediately after the failing syscall, where errno is guaranteed to belong to it.
The sync command path disables a proxy after rtpengine_retr failed attempts. An attempt only counts as failed when no matching reply arrives, but a late reply of a *previous* command (still queued in the socket buffer, e.g. after a timeout-triggered resend) also fails the cookie comparison - and such an attempt currently counts as a full failure, although the proxy just sent us a datagram. Track whether any data at all was received during the retries; if the node produced traffic, skip the disabling at the badproxy label: the proxy is demonstrably up, only its replies were stale. A node that stays completely silent still gets disabled as before.
An async command timing out says that one command did not get its reply in time - a single lost/late datagram is enough. That is no evidence that the node is down: it may just be slow, busy, or the reply may have been dropped on a congested link. Disabling the node for rtpengine_disable_tout punishes a healthy node for one missing datagram, and when a burst of timeouts hits (e.g. brief network issues), all nodes get disabled at once, dropping the whole platform to 'no available proxies'. Node liveness is better left to the existing probes: the periodic rtpe_test() ping (when ping/timer logic is enabled) and the sync command path, which only disables a node that stays completely silent while retrying.
Author
|
Closing in favor of a follow-up PR containing only the clear-cut bug fixes (crash/heap-corruption class), split out from the behavioral changes discussed here. |
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
2 participants
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
Five fixes for the
rtpenginemodule (plus one in its bundledbencodehelper), all found and verified while hardening a production OpenSIPS 3.6 platform. The overarching theme: the node-disabling logic is too eager — it takes the proxy down for conditions that are not the proxy's fault, and in the worst case a single local resource shortage flags every node as down at once.Each commit is self-contained and can be reviewed/cherry-picked independently.
bencode: fully initialize the buffer before the first allocationIf
__bencode_piece_new()fails (pkg memory exhausted),bencode_buffer_init()returns -1 having only assignedbuf->pieces—free_listanderrorkeep their previous (stack garbage) values. Callers cannot distinguish the failure stage and routinely runbencode_buffer_free(), whose first statement dereferencesbuf->free_list: undefined behaviour (call through a garbage pointer). Zeroing the struct up front makes the failure path a safe no-op free; the success path assigns all fields explicitly, so behaviour is unchanged.rtpengine: close the actual fd, not the node index, on reply read errorRTPE_IO_ERROR_CLOSE(param->node->idx)expands (on its EPIPE/EBADF branch) toclose(idx)— closing whichever descriptor happens to carry the node's array index number (for the first nodes in a set: one of the stdio fds) — and then writes-1intonode->idx, making every laterrtpe_socks[node->idx]an out-of-bounds array access. The fd that actually failed is the per-command socket created bystart_async_send_rtpe_command().rtpengine: do not disable nodes on local resource errorsAn async command builds a fresh UDP socket per command;
connect()on it makes the kernel autobind a local source port fromip_local_port_range(shared between TCP and UDP). Once that pool runs out,connect()fails withEAGAIN, andsocket()may fail withEMFILE/ENFILE/ENOMEM. These are local machine conditions, yet the code jumped tobadproxyand disabled the node. With the pool exhausted, every command towards every node fails at once — a single local resource shortage flags all nodes as down, while the long-livedrtpe_socks[]probes (already bound, no new port needed) keep succeeding, so the nodes flip between enabled and disabled until the process is restarted. We hit exactly this in production (log signature:can't connect to RTP proxy udp:... (11:Resource temporarily unavailable)immediately followed bydisable itfor every node). Local errnos (EAGAIN,EADDRNOTAVAIL,ENOBUFS,ENOMEM,EMFILE,ENFILE) now only release the fd; errors that do reflect node/network reachability (ECONNREFUSED,ENETUNREACH,EPIPE, ...) still disable the node as before. The check is only performed immediately after the failing syscall.rtpengine: do not disable a node that is proven alive by its dataIn the sync command path, a late reply of a previous command (still queued in the socket buffer) fails the cookie comparison and counts as a full retry failure — although the proxy just sent us a datagram. The fix tracks whether any data was received during the retries; a node that produced traffic keeps its enabled state, a completely silent node is still disabled as before.
rtpengine: do not disable a node on an async reply timeoutAn async timeout means one command did not get its reply in time — a single lost/late datagram is enough. That is not evidence the node is down, and a burst of timeouts (brief network issues) would disable all nodes at once. Node liveness is better left to the existing probes: the periodic
rtpe_test()ping and the sync path, which only disables a node that stays completely silent while retrying.Testing
Built and running on a production OpenSIPS 3.6.9 deployment (Debian 12, ~10 rtpengine nodes behind it); the local-resource-error path was exercised by the production incident described above — after the fix, the same port-exhaustion condition only fails individual commands while all nodes stay enabled.