Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preventively clear blocked signals in case they were inherited by the… #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SSSSeb
Copy link

@SSSSeb SSSSeb commented Jul 13, 2022

… launcher process

lbzip2 requires a set of signals to properly work. (SIGUSR2 is one of them)

If the caller process of lbzip2 has blocked some of those signals then the blocked list is inherited and lbzip2 will not work.

This happens for example if you invoke lbzip2 from inside an RPM for CENTOS8 or SLES15SP2.

This patch is simply clearing the way (at the process level in setup_signals) to prepare for a functionnal lbzip2 whatever the caller status is in term of blocked signals.

replacing lbzip2 by bunzip2 in my use-case showed that only lbzip2 was impacted, very likely because lbzip2 only requires such requirements on signals.

I believe it is wrong that RPM is blocking signals but I think this is good to clear the way. When not having this patch, lbzip2 would uncompress the tarball and would hang forever (because the SIGUSR2 signal required was blocked between the main thread and the workers thread).

hope this helps.

@tansy
Copy link

tansy commented Oct 5, 2022

How can one emulate it to check it?

@SSSSeb
Copy link
Author

SSSSeb commented Oct 6, 2022

You could recompile the following and use it to launch an lbzip2 decompression. This will stall forever if you use lbzip2 master branch and should proceed with my patch.

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>

void main(int argc, char**argv)
{
sigset_t myfullset;

if (sigfillset(&myfullset)) {
fprintf(stderr,"error filling set\n");
exit(1);
}

if (sigprocmask(SIG_BLOCK,&myfullset,NULL)) {
fprintf(stderr,"error blocking all signals with sigprocmask\n");
exit(1);
}
system(argv[1]);
}

@tansy
Copy link

tansy commented Oct 7, 2022

I tested it and it does work.

Do you have to block all signals? I guess not.

And if RPM blocks some of them does it do it for a reason and if so then for what reason?
Shall it be asked RPM devs?

@SSSSeb
Copy link
Author

SSSSeb commented Oct 10, 2022

I tested it and it does work.

Do you have to block all signals? I guess not.

probably not, but this was my reproducer of the RPM behavior (I used strace and found that they block all signals).

And if RPM blocks some of them does it do it for a reason and if so then for what reason? Shall it be asked RPM devs?

I've looked in the RPM source code tree and they have removed this signal blocking a long time ago but main linux distros are not up-to-date with upstream rpm devs

My conclusion is: if the signal is blocked then lbzip2 will not work at all, so we should unblock it or test and do an early exit. Stalling forever is not good for lbzip2 compared to all others. my 2 cents.

@tansy
Copy link

tansy commented Oct 13, 2022

Thanks for the answer.

Regarding the code, IMO the third line of the comment (`such as rpm tools as of CENTOS8/SLES15') is unnecessary detail that shouldn't be in code. It would better be in description, even could find a place in ChangeLog but code is not place for such information.

@SSSSeb
Copy link
Author

SSSSeb commented Oct 24, 2022

I agree with you, I should not have put those comments in the code, they do not belong here.

@SSSSeb
Copy link
Author

SSSSeb commented Oct 24, 2022

what should I do next, remove the line and propose a commit wihtout the extra comment line instead or do you want to do that ?

@SSSSeb SSSSeb force-pushed the clear_signals_before_start branch from b11b442 to 00216b2 Compare October 24, 2022 11:30
@SSSSeb
Copy link
Author

SSSSeb commented Oct 24, 2022

just in case, I pushed force a different commit to accomodate better your comment.

@tansy
Copy link

tansy commented Oct 30, 2022

I wonder if it will ever be done as this project seems abandoned. Last @kjn's activity was in 2018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants