qrouting: fix multiple bugs across the module#3861
Open
abdoulosseni wants to merge 1 commit intoOpenSIPS:masterfrom
Open
qrouting: fix multiple bugs across the module#3861abdoulosseni wants to merge 1 commit intoOpenSIPS:masterfrom
abdoulosseni wants to merge 1 commit intoOpenSIPS:masterfrom
Conversation
Fix the following issues in the qrouting module:
1. MI "enable_dst" command was calling mi_qr_disable_dst_* functions
instead of mi_qr_enable_dst_*, effectively disabling destinations
when users intended to enable them (copy-paste error).
2. Read lock leak in qr_score_grp() when a gateway is disabled and
not dirty: the else branch was missing a lock_stop_read() call.
3. update_grp_stats() took its qr_grp_t argument by value, so the
QR_STATUS_DIRTY flag was written to a stack copy and never applied
to the actual group, causing stale cached scores.
4. qr_set_dst_state() always locked dst->gw->ref_lock regardless of
destination type. For QR_DST_GRP destinations, this reinterprets
the grp union member as a gw pointer (undefined behavior). Now
locks dst->grp.ref_lock for carrier destinations.
5. qr_weight_based_sort() swapped destination indices but not their
corresponding scores, causing wrong weights after the first
iteration.
6. Memory leak of dialog_prop (shm) on three error paths in
qr_check_reply_tmcb() after successful allocation.
7. NULL pointer dereference on *qr_main_list in MI status/enable/
disable functions and w_qr_set_dst_state() when called before
drouting has loaded data.
8. DB connection leak in qr_check_db(): the handle was not closed
on the capability check and table version check error paths.
9. Division by zero when sampling_interval is set to 0: added
parameter validation.
10. Memory leak in qr_reload() on shm_realloc failure: the old profs
pointer was overwritten with NULL (losing the original block),
and the DB result set was not freed. Now uses a temporary variable
to preserve the original pointer and properly cleans up.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
Fix 10 bugs found in the qrouting module:
Critical (incorrect behavior):
enable_dstdisables instead of enabling: copy-paste error caused the MI command to callmi_qr_disable_dst_*functions instead ofmi_qr_enable_dst_*qr_score_grp(): missinglock_stop_read()when gateway is disabled and not dirty, causing progressive deadlockupdate_grp_stats()pass-by-value:QR_STATUS_DIRTYflag written to stack copy, never applied to actual group — carrier scores remain staleqr_set_dst_state():dst->gw->ref_lockaccessed unconditionally, but forQR_DST_GRPdestinations this is undefined behavior (reinterpretsgrpunion member asgwpointer)qr_weight_based_sort()swappeddsts[]but notscores[], causing wrong weights after first iterationResource leaks & crashes:
dialog_propshm leak: 3 error paths inqr_check_reply_tmcb()aftershm_mallocdon't free on failure*qr_main_list: MI status/enable/disable functions andw_qr_set_dst_state()dereference without NULL check, crashing if called before drouting loadsqr_check_db(): handle not closed on capability/version check failuressampling_interval=0not validated, causes crash athistory_span * 60 / sampling_intervalqr_reload():shm_reallocfailure overwritesprofswith NULL (losing original block) and doesn't free DB result set; now uses temp variableTest plan
enable_dstcommand correctly re-enables a previously disabled gatewaydisable_dststill works as expectedqr_reloadbehavior under memory pressuresampling_interval=0(should fail cleanly)🤖 Generated with Claude Code