[kamailio/kamailio] core: main - Protect async-signal-unsafe code in sig_usr() with SIG_DEBUG (#2127)
#### Pre-Submission Checklist - [ ] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [ ] 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 - [ ] 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: - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
The sig_usr() signal handler contains debug code to print memory usage and statistics that is not async-signal-safe, which can cause crashes or misbehavior. This code which only affects explicit SIGUSR1 signals on children processes, and is not guarded with the SIG_DEBUG macro. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2127
-- Commit Summary --
* core: main - Protect async-signal-unsafe code in sig_usr() with SIG_DEBUG
-- File Changes --
M src/main.c (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2127.patch https://github.com/kamailio/kamailio/pull/2127.diff
Thank you for the pull request. You are right that its not nice if this error happens during a shut-down. So far we did not got many reports for this issue.
If I understand the patch correctly it would be disable the possibility to do the memory logging during shutdown without a recompilation. This memory debugging is a great help during complicated issues reported from our users.
For this reasons we enable some other settings as well in our source code by default, e.g. compile in debugging mode and without advanced optimization. So I don't want to merge it right now without proper consideration the pros and cons.
Lets wait for the opinion from other developers on this one.
Hey @henningw, actually it should not really remove the reporting during shutdown, as that is usually (when not using -D) handled by the handle_sigs() on the main_loop(). This pretty much affects only the children that get signals.
This still leaves the -D usage on the main process as unsafe, but I guess it's a similar compromise to the one of rebuilding with unsafe signal debugging enabled.
Thank you for the clarification, I will have another look to it in the next days.
@henningw ping about this one
@linuxmaniac - thanks for the reminder. I also looked at this PR a while ago, but forgot to comment.
The version of kamailio is not provided to match the source code line from backtrace, but it seems to be related to printing the memory usage logs at shutdown.
Based on my understanding so far, I do not see the usefulness of this PR at all. The memory status report/summary is printed only if some global parameters are set, respectively `memlog` less or equal than `debug` and `mem_summary` different than `0`.
in my opinion, there are enough options to disable printing the memory logs at the shutdown, if one faces problems with them. Actually they are not printed using the default config, one needs to change those params to get them printed.
Anyhow, if I misunderstood, definitely I do not want a compile flag (macro) to disable/enable shutdown reports, it has to be a global param. It is not convenient to recompile the sources if it is need of more info for troubleshooting.
@miconda I could try to find the specific version for that backtrace, but this is the Sipwise downstream version with all our patches so this might not give you the expected line numbers when compared to a pristine upstream codebase anyway.
Re the usefulness, the first patch makes the code consistent so that when you disable SIG_DEBUG and enable PKG_MALLOC you still get safe behavior, matching the existing code dealing with PKG_MALLOC in the signal handler. For the default change I could take a look at making it run-time configurable, but IMO it should still default to off, but then see the next item.
Re the shutdown reports, please see my reply to @henningw https://github.com/kamailio/kamailio/pull/2127#issuecomment-554650695.
The first patch it is ok, SIG_DEBUG is defined by default.
Lately some of the devs put efforts in eliminating compile time macro switches that were not changed for very long time, to make the code easier to follow. Anyhow, I am fine with the first commit, if it is useful.
But turning this off by default at compile time is not ok, it is needed for tracking memory leaks in PKG and if someone reports it, likely it has it installed with default compile options. Asking to recompile with new flags is not convenient at all, production systems are typically not configured for building packages. And again, the PKG report should not be printed if the global parameters listed above were not changed.
The signal handler of the main process is printing only PKG reports for itself, the main process does not print PKG reports for child processes.
I incorporated the first commit in this PR in one (31f0612) I did to add debug messages on other signals for coherence.
The second one that disables debug messages on shutdown by default won't be merged as it is, because getting pkg memory reports is very useful and should be possible without recompiling kamailio, even at the risk of a crash during shut down. Even more, getting pks report can be controlled via memlog global parameter, which can also be changed at runtime without restart.
If one wants to make a variant based on a global parameter instead on disabling at compile time, then make another pull request.
Closed #2127.
-
Daniel-Constantin Mierla -
Guillem Jover
-
Henning Westerholt
-
Victor Seva