kdesktop: Add missing initialization of sigset_t in kdesktop_lock. #42

Merged
MicheleC merged 1 commits from bug/3025 into master 5 years ago
Owner

Mike Bird in bug 3025 reported missing initialization. Although the correction seems simple, there is one more thing to discuss.

The new_mask variable is used for the first time in sigprocmask on line 246. There is no initialization here, as Mike pointed out and as is fixed in proposed commit.

Then new_mask variable is newly initialized using sigemptyset on line 426 and subsequent calls sigaddset and then used in sigprocmask on line 449.

Until this point, everything was fine. However, then the new_mask variable is reused in the sigprocmask on the line 460. And here is the question of whether a mask set from line 426 should be used in this call or whether it should be reinitialized by sigfillset(…)? Because the comment mentions blocking all signals, just like in the first case.

Mike Bird in bug [3025](http://bugs.pearsoncomputing.net/show_bug.cgi?id=3025) reported missing initialization. Although the correction seems simple, there is one more thing to discuss. The `new_mask` variable is used for the first time in [`sigprocmask` on line 246](src/branch/master/kdesktop/lock/main.cc#L246). There is no initialization here, as Mike pointed out and as is fixed in proposed commit. Then `new_mask` variable is newly initialized using [`sigemptyset` on line 426](src/branch/master/kdesktop/lock/main.cc#L426) and subsequent calls `sigaddset` and then used in [`sigprocmask` on line 449](src/branch/master/kdesktop/lock/main.cc#L449). Until this point, everything was fine. However, then the `new_mask` variable is reused in the [`sigprocmask` on the line 460](src/branch/master/kdesktop/lock/main.cc#L460). And here is the question of whether a mask set from line 426 should be used in this call or whether it should be reinitialized by `sigfillset(…)`? Because the comment mentions blocking **all** signals, just like in the first case.
SlavekB added this to the R14.0.7 release milestone 5 years ago
SlavekB added the PR/rfc label 5 years ago
Owner

I have checked the code too. It seems we need to initialize again new_mask with sigfillset() before the last call, if we want to block all signals as the comment says. Not sure what difference it will make, so we should test before merging.

I have checked the code too. It seems we need to initialize again new_mask with sigfillset() before the last call, if we want to block all signals as the comment says. Not sure what difference it will make, so we should test before merging.
Poster
Owner

Note: In a comment 5 on bugzilla I mentioned the important fact that in the stable branch used by Mike, calls sigprocmask are different and there is no call without initialization.

In any case, sigfillset(&new_mask); is probably also to be used for the stable branch as we discussed above.

Note: In a [comment 5](http://bugs.pearsoncomputing.net/show_bug.cgi?id=3025#c5) on bugzilla I mentioned the important fact that in the stable branch used by Mike, calls `sigprocmask` are different and there is no call without initialization. In any case, `sigfillset(&new_mask);` is probably also to be used for the stable branch as we discussed above.
Owner

I have tested this PR in R14.1.0-dev and I see major issues with blocking all signals.

  1. screensaver becomes unresponsive: moving or clicking the mouse does not show the unlock dialog immediately but need to wait for several seconds

  2. after unlocking the screen, need to wait several seconds for the desktop to be useful

I repeated the tests on 2 different users and switching back-and-forth with the master version. Each time I tested the PR I faced the same problems, while using the master version there were no issues.

It seems blocking all signals is probably wrong, and thinking about it, it sort of make sense, since we are also blocking the response to IO events such as the mouse. In fact I now wonder how it could still work with all the signal blocked 😧

I have tested this PR in R14.1.0-dev and I see major issues with blocking all signals. 1. screensaver becomes unresponsive: moving or clicking the mouse does not show the unlock dialog immediately but need to wait for several seconds 2. after unlocking the screen, need to wait several seconds for the desktop to be useful I repeated the tests on 2 different users and switching back-and-forth with the master version. Each time I tested the PR I faced the same problems, while using the master version there were no issues. It seems blocking all signals is probably wrong, and thinking about it, it sort of make sense, since we are also blocking the response to IO events such as the mouse. In fact I now wonder how it could still work with all the signal blocked :anguished:
Poster
Owner

Yes, I can confirm that the behavior on R14.0.x is also significantly worse when all signals are blocked. The unlock window has a long delay and the screen saver is not properly deleted. When the unlock window is closed (without unlock), the screen saver is not restarted.

Note: Only the second part of the patch is applied because the first is not valid for r14.0.x branch.

Yes, I can confirm that the behavior on R14.0.x is also significantly worse when all signals are blocked. The unlock window has a long delay and the screen saver is not properly deleted. When the unlock window is closed (without unlock), the screen saver is not restarted. Note: Only the second part of the patch is applied because the first is not valid for r14.0.x branch.
SlavekB changed title from kdesktop: Add missing initialization of sigset_t in kdesktop_lock. to WIP: kdesktop: Add missing initialization of sigset_t in kdesktop_lock. 5 years ago
Owner

I have updated Slavek's commit removing the last sigfillset() call. Tested on R14.1.0-dev, seems to work fine.

I have updated Slavek's commit removing the last sigfillset() call. Tested on R14.1.0-dev, seems to work fine.
MicheleC changed title from WIP: kdesktop: Add missing initialization of sigset_t in kdesktop_lock. to kdesktop: Add missing initialization of sigset_t in kdesktop_lock. 5 years ago
SlavekB removed the PR/rfc label 5 years ago
MicheleC closed this pull request 5 years ago
MicheleC deleted branch bug/3025 5 years ago
MicheleC modified the milestone from R14.0.7 release to R14.1.0 release 5 years ago
Owner

Changed to R14.1.0 milestone, since the changes in this PR are only valid for the master branch.

Changed to R14.1.0 milestone, since the changes in this PR are only valid for the master branch.
The pull request has been merged as e47bab3fd8.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdebase#42
Loading…
There is no content yet.