NormB left a comment (kamailio/kamailio#4644)
Good points, thanks for the review.
**Dead code removal** -- agreed, updated patch removes the path 3 block
entirely.
**kill_transaction() vs t_release_transaction()** -- safe. `kill_transaction()`
checks `has_reply_route()` and `FL_FINAL_REPLY` first (t_funcs.c), and if a
final reply was already sent, it degrades to `t_release_transaction()` --
identical to what path 3 did. When no final reply was sent (our case: all
branches dropped), the UAC is waiting with no response, so generating a 500 is
the correct behavior.
**On the explicit `||` alternative** -- the problem is wider than just
`REQ_FWDED`. Because `set_kr()` uses `|=` on process-local state, any
combination of flags can accumulate. The path 3 mask itself proves this:
```c
(kr & ~(REQ_RLSD | REQ_RPLD | REQ_ERR_DELAYED | REQ_FWDED))
```
This explicitly *excludes* those flags from triggering cleanup, so path 3 never
fires for any of these combinations either:
- `ERR_DELAYED | FWDED` (17): `17 & ~23 = 0` → path 3 skips → **leak**
- `ERR_DELAYED | RPLD` (18): `18 & ~23 = 0` → path 3 skips → **leak**
- `ERR_DELAYED | RLSD` (20): `20 & ~23 = 0` → path 3 skips → **leak**
Path 3 only ever caught `REQ_EXIST` contamination (`kr & ~23 != 0`), which is
unlikely in practice. The `||` approach would fix one known case; the `&` test
handles all of them and is robust against future flag additions.
Tested the updated patch (bitwise `&` + path 3 removed) on 6.2.0-dev0 master
(4767914):
| Test | Patch | current | 5xx | freed | Verdict |
|------|-------|---------|-----|-------|---------|
| Baseline (no patch) | no | 500 | 0 | 500/1000 | **LEAK** |
| Updated patch | yes | **0** | 500 | 2000/2000 | **OK** |
| Updated patch + `delayed_reply=0` | yes | **0** | 500 | 2000/2000 | **OK** |
Same results as the original 10-test matrix across 6.0.3/6.1.0/master --
removing path 3 has no effect since it was already unreachable after the `&`
change.
Commit updated with path 3 removed and expanded commit message.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4644#issuecomment-4068261364
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4644/[email protected]>
_______________________________________________
Kamailio - Development Mailing List -- [email protected]
To unsubscribe send an email to [email protected]
Important: keep the mailing list in the recipients, do not reply only to the
sender!
