[sr-dev] [kamailio/kamailio] modules/snmpstats: exit with `ksr_exit()` instead of standard `exit()` (PR #3993)

The former respects the `--atexit=no` from Kamailio command line. See note on `atexit` in TLS module "Overview". <!-- Kamailio Pull Request Template --> <!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception --> #### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated) #### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality) #### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number) #### Description <!-- Describe your changes in detail --> I'm using `tls` module along with some other modules. On Kamailio termination there are a lot of "freeing already freed pointer" messages from `qm_free()`. Also there may be a corrupted memory report (followed by `abort()`) from `qm_debug_check_frag()`. I'm aware of `--atexit=no` required for `tls`, and Kamailio is indeed started as `kamailio --atexit=no -m 16 -n 1 -w . -f some.cfg`. The `tls` module is loaded and configured something like this: ``` disable_core_dump=no debug=2 memdbg=2 # mem_safety=0 enable_tls=yes listen=tls:127.0.0.1:5060 mpath="lib64/kamailio/modules" loadmodule "tls.so" modparam("tls", "init_mode", 1) modparam("tls", "config", "tls.cfg"); loadmodule "snmpstats.so" modparam("snmpstats", "sipEntityType", "proxyServer") modparam("snmpstats", "snmpgetPath", "/usr/bin/") modparam("snmpstats", "snmpCommunity", "MySNMPComunity") modparam("snmpstats", "snmpVersion", "2c") loadmodule "xlog.so" route { xlog("L_NOTICE", "Hello\n"); } ``` Running Kamailio and then stopping it with `SIGTERM` yields the following. Some blocks of shared memory are allocated by the main process (PID 62575). E.g., one of the blocks: ``` 0(62575) INFO: <core> [core/mem/q_malloc.c:429]: qm_malloc(): qm_malloc(0x7facc5dc0000, 8) returns address 0x7facc5e55810 frag. 0x7facc5e557d8 (size=8) on 1 -th hit ``` On termination, this address is freed by another process (PID 62583). Somewhere after "sig_usr(): signal 15 received" message there's the following entry in the log: ``` 5(62583) INFO: <core> [core/mem/q_malloc.c:495]: qm_free(): qm_free(0x7facc5dc0000, 0x7facc5e55810), called from tls: tls_init.c: ser_free(400) ``` Then later the same address is freed again, by the main process (PID 62575). That's due to explicit call to `OPENSSL_cleanup()` from `tls_h_mod_destroy_f()` in `tls` module: ``` 0(62575) INFO: tls [tls_init.c:1063]: tls_h_mod_destroy_f(): executing openssl v1.1+ cleanup 0(62575) INFO: <core> [core/mem/q_malloc.c:495]: qm_free(): qm_free(0x7facc5dc0000, 0x7facc5e55810), called from tls: tls_init.c: ser_free(400) 0(62575) CRITICAL: <core> [core/mem/q_malloc.c:535]: qm_free(): BUG: freeing already freed pointer (0x7facc5e55810), called from tls: tls_init.c: ser_free(400), first free tls: tls_init.c: ser_free(400) - ignoring ``` It depends on initial conditions (which modules are loaded, what is the configuration, etc.), but sometimes `qm_debug_check_frag()` detects memory corruption and calls `abort()`: ``` 0(62575) CRITICAL: <core> [core/mem/q_malloc.c:126]: qm_debug_check_frag(): BUG: qm: fragm. 0x7facc5e891e0 (address 0x7facc5e89218) beginning overwritten (7facc5e85498)! Memory allocator was called from tls: tls_init.c:400. Fragment marked by :140376711102467. Exec from core/mem/q_malloc.c:526. ``` The output of `kamcmd core.ps` suggests that the offending process (PID 62583) is related to SNMP: ``` 62575 main process - attendant … 62583 SNMP AgentX ``` The "SNMP AgentX" process in `snmpstats` overrides some signal handlers (including the `SIGTERM` one) and calls `exit()`: ``` /*! Creates a child that will become the AgentX sub-agent. The child will * insulate itself from the rest of Kamailio by overriding most of signal * handlers. */ void agentx_child(int rank) { … /* Setup a SIGTERM handler */ sigfillset(&new_sigterm_handler.sa_mask); new_sigterm_handler.sa_flags = 0; new_sigterm_handler.sa_handler = sigterm_handler; sigaction(SIGTERM, &new_sigterm_handler, NULL); … } ``` ``` static void sigterm_handler(int signal) { /* Just exit. The master agent will clean everything up for us */ exit(0); } ``` Looks like this `exit()` triggers an extra call to `OPENSSL_cleanup`, implicitly armed by OpenSSL itself on startup. So, the OpenSSL cleanup gets called twice. As for now I've fixed the issue by replacing this `exit(0)` with `ksr_exit(0)` which respects the `--atexit=no` command line option. But I wonder is it a right way to do it. As far as I can see, code in `src/modules/` ignores `ksr_exit()` and uses plain `exit()`. Is it for a reason? Shouldn't modules also exit with `ksr_exit`? You can view, comment on, or merge this pull request online at: https://github.com/kamailio/kamailio/pull/3993 -- Commit Summary -- * modules/snmpstats: exit with ksr_exit() instead of standard exit() -- File Changes -- M src/modules/snmpstats/snmpstats.c (5) M src/modules/snmpstats/sub_agent.c (5) -- Patch Links -- https://github.com/kamailio/kamailio/pull/3993.patch https://github.com/kamailio/kamailio/pull/3993.diff -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3993 You are receiving this because you are subscribed to this thread. Message ID: <kamailio/kamailio/pull/3993(a)github.com>