[kamailio/kamailio] core: main - Protect async-signal-unsafe code in sig_usr() with SIG_DEBUG (#2127)
13 Nov
2019
13 Nov
'19
11:43 p.m.
#### 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
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127
15 Nov
15 Nov
11:03 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-554508038
16 Nov
16 Nov
7:11 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-554650695
20 Nov
20 Nov
10:23 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
Thank you for the clarification, I will have another look to it in the next days.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-556277199
15 Jun
15 Jun
8:23 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
@henningw ping about this one
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-644265328
10:13 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
@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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-644325130
18 Jun
18 Jun
6:57 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
@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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-646120947
7:30 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-646147232
2 Dec
2 Dec
6:28 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#issuecomment-737303062
6:28 p.m.
New subject: [kamailio/kamailio] core: Improve handling of async-signal-unsafe code in signal handler (#2127)
Closed #2127.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2127#event-4062954454
1451
days inactive
1836
days old
9 comments
4 participants
participants (4)
-
Daniel-Constantin Mierla -
Guillem Jover
-
Henning Westerholt
-
Victor Seva