Conversation
If __bencode_piece_new() fails (pkg memory exhausted), bencode_buffer_init() returns -1 having only assigned buf->pieces, leaving free_list and error with their previous (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.
When a delete reply carrying statistics is processed for a context
that already has stats cached, the code did:
rtpe_stats_free(ctx->stats);
pkg_free(&(ctx->stats->buf));
but buf is an embedded bencode_buffer_t inside struct rtpe_stats -
the second member, after dict - so &ctx->stats->buf is an interior
pointer, not the address pkg_malloc() returned. Freeing it corrupts
the pkg allocator's free lists.
On top of that, ctx->stats itself was not NULLed afterwards, so the
code right below kept using it (writing through ctx->stats->buf =
...) - a dangling pointer write into memory handed back to the
allocator.
Keep the rtpe_stats_free() call (it releases the json string and the
buffer's inner pieces, so the struct can be safely reused once the
three fields are overwritten just below) and drop the bogus
pkg_free(): rtpe_ctx_free() eventually frees the struct itself using
its real base address.
Triggers on the second delete-with-stats reply processed on the same
message context, e.g. a call re-invited through rtpengine again.
Two fd lifecycle bugs in resume_async_send_rtpe_command():
1. The unix branch closes the fd right after read(). But every return
path of this function sets async_status = ASYNC_DONE_CLOSE_FD, so
the async framework (tm/async.c:180, async.c:242) closes the same
fd again right after the resume function returns - a plain double
close, which hits an unrelated descriptor whenever the fd number
gets reused in between.
2. The UDP error branch calls RTPE_IO_ERROR_CLOSE(param->node->idx).
The macro expands to close(_fd) followed by (_fd) = -1, so on its
EPIPE/EBADF branch it:
- closes() whatever descriptor happens to carry the node's array
index number - for the first nodes of a set that is one of the
stdio descriptors (idx 0, 1 or 2);
- overwrites the shared-memory node->idx with -1, turning every
later rtpe_socks[node->idx] access into an out-of-bounds array
access.
Both are fixed by dropping the manual close attempts and leaving the
fd exclusively to the framework, which already guarantees the close
through ASYNC_DONE_CLOSE_FD on all paths.
rtpe_function_call_async() does not check the bencbuf allocation result before passing it on: on pkg memory exhaustion the very first touch of the buffer (bencode_buffer_init()) dereferences the NULL pointer and crashes the worker. No resources have been allocated at that point, so a plain early return is sufficient.
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
Four clear-cut bug fixes for the
rtpenginemodule and its bundledbencodehelper, found while hardening a production OpenSIPS deployment. Each commit is self-contained; none of them changes any behaviour apart from removing the crash/corruption itself.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 (garbage) values. Callers, which cannot distinguish the failure stage, routinely runbencode_buffer_free()on the returned error, and the free routine starts by dereferencingbuf->free_list: undefined behaviour (reading a garbage pointer and calling through it). Zeroing the struct up front makes the failure path a safely freeable empty object; the success path assigns all three fields explicitly, so nothing changes there.rtpengine: fix heap corruption when caching delete statisticsbufis an embeddedbencode_buffer_tinsidestruct rtpe_stats— the second member, afterdict— so&ctx->stats->bufis an interior pointer, not the addresspkg_malloc()returned. Freeing it corrupts the pkg allocator's free lists. On top of that,ctx->statsitself was not NULLed afterwards, so the code right below kept writing through a dangling pointer (ctx->stats->buf = ...). The fix keepsrtpe_stats_free()(which releases the json string and the buffer's inner pieces, making the struct safely reusable once the three fields are overwritten just below) and drops the boguspkg_free();rtpe_ctx_free()eventually frees the struct using its real base address. Triggers on the second delete-with-stats reply processed on the same message context.rtpengine: close the async reply fd exactly once, and not the node indexTwo fd lifecycle bugs in
resume_async_send_rtpe_command():read(). But every return path setsasync_status = ASYNC_DONE_CLOSE_FD, so the async framework (tm/async.c:180,async.c:242) closes the same fd again once the resume function returns — a double close that hits an unrelated descriptor whenever the fd number is reused in between.RTPE_IO_ERROR_CLOSE(param->node->idx). The macro expands toclose(_fd)followed by(_fd) = -1, so on its EPIPE/EBADF branch it (a) closes whichever descriptor happens to carry the node's array index number — for the first nodes of a set that is one of the stdio descriptors — and (b) writes-1into the shared-memorynode->idx, turning every laterrtpe_socks[node->idx]access into an out-of-bounds array access.Both fixed by dropping the manual close attempts and leaving the fd exclusively to the framework, which already guarantees the close through
ASYNC_DONE_CLOSE_FDon all paths.rtpengine: check the pkg_malloc() result of the async bufferrtpe_function_call_async()never checks thebencbufallocation result: on pkg memory exhaustion, the very first touch of the buffer (bencode_buffer_init()) dereferences the NULL pointer and crashes the worker. Nothing else is allocated at that point, so an early return is sufficient.Testing
Built and running on a production 3.6.x deployment (as a source overlay on the 3.6.9 tag); the heap-corruption path was identified through code audit of the stats caching flow, the fd and NULL-check paths through the same audit plus runtime behaviour on a ~10 node rtpengine farm.