Bug 317496 - Fix signal handler
Summary: Fix signal handler
Status: RESOLVED LATER
Alias: None
Product: ksmserver
Classification: Unmaintained
Component: general (show other bugs)
Version: 4.10.1
Platform: Other All
: NOR normal
Target Milestone: ---
Assignee: Lubos Lunak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-28 15:50 UTC by Markus Elfring
Modified: 2023-05-17 15:45 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Elfring 2013-03-28 15:50:34 UTC
I find the implementation of the function "sighandler" questionable.
https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/c7fc5ed96143d91e16787a7e231f2c87e9fa2df5/entry/ksmserver/server.cpp#L527

1. No meaningful data processing is performed for "SIGHUP".
   Should any configuration file be reread here?

2. How do you think about to perform program clean-up outside the signal handling context?
   https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
   http://stackoverflow.com/questions/4812491/how-to-deal-with-sigint
Comment 1 Andrew Crouthamel 2018-11-10 03:23:49 UTC
Dear Bug Submitter,

This bug has been stagnant for a long time. Could you help us out and re-test if the bug is valid in the latest version? I am setting the status to NEEDSINFO pending your response, please change the Status back to REPORTED when you respond.

Thank you for helping us make KDE software even better for everyone!
Comment 2 Markus Elfring 2018-11-10 12:18:59 UTC
(In reply to Andrew Crouthamel from comment #1)

The software situation was improved a bit with the commit “Sanitize signal handling in ksmserver” from 2018-04-13.
https://github.com/KDE/plasma-workspace/commit/eb6e261979842e158dd48b1b1a32d3ae653c4389

I find that a few software development concerns remain.

* The data type “sig_atomic_t” should be used for the variable “wake_up_socket”.
* Is the data processing for “SIGHUP” still questionable?
Comment 3 David Edmundson 2018-11-10 17:37:49 UTC
>* The data type “sig_atomic_t” should be used for the variable “wake_up_socket”.

Technically yes (or std::atomic if we want to be modern), though practically we're not not using that variable in parallel in the main thread except in cleanup.

I'd happily accept a patch, or I can do it.

>* Is the data processing for “SIGHUP” still questionable?

Doesn't seem to do anything useful, it was there since the initial commit. I guess the author probably intended to do something.

Using SIGHUP for config reloading isn't something we do in any KDE apps.
Comment 4 Markus Elfring 2018-11-10 18:03:09 UTC
(In reply to David Edmundson from comment #3)

* Will a socket descriptor fit into the portable value range for the data type “sig_atomic_t”?

* Will the support for system configuration reloading be added to any more software applications?
Comment 5 Justin Zobel 2022-12-02 01:23:04 UTC
Thank you for reporting this issue in KDE software. As it has been a while since this issue was reported, can we please ask you to see if you can reproduce the issue with a recent software version?

If you can reproduce the issue, please change the status to "REPORTED" when replying. Thank you!
Comment 6 Markus Elfring 2022-12-02 17:48:26 UTC
(In reply to Justin Zobel from comment #5)
Would you like to help any more with a proper source code review?
https://github.com/KDE/plasma-workspace/blob/0ecf1cbfaa583f84a53e8710af6dcdb57bf81310/ksmserver/server.cpp#L436
Comment 7 Nate Graham 2023-05-17 15:45:00 UTC
Feel free to submit a merge request fixing this. Otherwise I suspect it will unfortunately not get done, due to lack of resourcing.