Since there is yet another part to discuss, and since TGW is more appropriate for discussing the proposed patch, there is a pull request in TGW:

https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/42

I may have been mistaken to suggest sigfillset.

There's another supposed block of all signals at line 460 but new_mask at this point contains only the five signals of interest.

I'm guessing that someone wrote the new_mask initialization starting at line 390 and the "(signalled_run == FALSE)" loop starting at line 433 and probably got them basically right, but there was a race condition before and after this code section.  So someone else added the "block all signals" and got it wrong.

Maybe the comments about blocking all signals really mean to block the five signals of interest.  Maybe all that is needed is for the existing initialization of new_mask to be moved before its first use, rather than using sigfillset which would lose any pre-existing signal mask without saving it.

If the code was really supposed to block all signals there would be no need to record orig_mask at line 449.

YMMV.  I've been staring at this file for days and there's still a lot I don't understand.

I noticed one more important thing - the part where new_mask is used without initialization is new change in master branch but not in stable branch. Therefore, the stable branch may not be affected by this issue.

> the part where new_mask is used without initialization is new change in master branch but not in stable branch.

Are you sure?

I was looking at code through: https://git.trinitydesktop.org/cgit/
but if there's a "git blame" function there I couldn't find it.  I did check the log there and didn't see anything relevant.

So I did "GIT_ASKPASS=echo git clone --recursive https://anonymous@scm.trinitydesktop.org/scm/git/tde" and ran "git blame" where I see the use of uninitialized new_mask in Tim's original check in of KDE 3.5:

b5cb2797d (Timothy Pearson 2015-09-21 15:39:58 -0500 242)               sigset_t new_mask;
b5cb2797d (Timothy Pearson 2015-09-21 15:39:58 -0500 243)               sigset_t orig_mask;
b5cb2797d (Timothy Pearson 2015-09-21 15:39:58 -0500 244)
b5cb2797d (Timothy Pearson 2015-09-21 15:39:58 -0500 245)               // Block reception
of all signals in this thread
b5cb2797d (Timothy Pearson 2015-09-21 15:39:58 -0500 246)               sigprocmask(SIG_BLO
CK, &new_mask, NULL);

Of course, if there's a race, then disabling signals in main is probably still racy.  They probably need to be disabled before this process is created, just as they are disabled before this process's own lock thread is created.

Meanwhile what got me here in the first place is trying to figure out why desktop_lock processes were crashing with multiple non-xinerama screens.

So we have two aspects: multiple non-xinerama screens and signal masking/races.  I don't know about you but I'm certainly well short of understanding exactly how to do them right.

Yes, I'm sure the code is different between master and r14.0.x branch.

You can look at the description in the pull-request and the attached patch that now addresses for the master branch both sigprocmask calls with the comment // Block reception of all signals in this thread

https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/42/files

While in r14.0.x a branch is only the second call that follows a code that sets values to new_mask. It is likely that sigfillset(& new_mask) should be used for this call, but it is certainly not a call without initialization, as in the case of a master branch.

See:

https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/src/branch/r14.0.x/kdesktop/lock/main.cc#L241
https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/src/branch/r14.0.x/kdesktop/lock/main.cc#L379
https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/src/branch/r14.0.x/kdesktop/lock/main.cc#L417
https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/src/branch/r14.0.x/kdesktop/lock/main.cc#L451

Thanks.  My mistake.  I had neglected to also switch the submodule to the r14.0.x branch.

BTW, are you deleting https://git.trinitydesktop.org/cgit/ or is it breaking?  Now when I click on a submodule e.g. tdebase I get no such file or directory.  Was OK a few days ago.

> While in r14.0.x a branch is only the second call that follows a code that sets values 
> to new_mask. It is likely that sigfillset(& new_mask) should be used for this call, but 
> it is certainly not a call without initialization, as in the case of a master branch.
I agree, looks like sigfillset is missing in r14.0.x branch too before the last call.

This bug report is my mistake.  I was looking at the wrong branch because I had neglected to switch the submodule to the r14.0.x branch.  (I dislike submodules!)

I believe Slávek's proposed patch is similarly faulty.

Tim appears to have fixed this problem in the r14.0.x back in 2015.

There's still a problem with desktop_lock on multiple non-xinerama screens but this is not the solution.

Would Slávek like to mark this as RESOLVED/INVALID or similar?

We have merged PR #42 which initializes new_mask correctly.
https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/42