Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4065 closed defect (fixed)

block all signals in child threads

Reported by: fdupont Owned by: fdupont
Priority: very high Milestone: Kea1.0-beta
Component: libutil Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Signals must be delivered only to the main thread so the thread util library should block all signals when a new (child) thread is created.

Subtickets

Change History (10)

comment:1 Changed 4 years ago by marcin

This is something that will be mostly covered in other tickets related to leases expiration, so I am not sure having this separate ticket is really valid.

comment:2 Changed 4 years ago by fdupont

It is an util library bug so IMHO it has to be fixed independently to anything else.

comment:3 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from new to reviewing

Seems to have no bad side effects so please review. BTW there are still some works to do in recent additions to util, cf #4060.

comment:4 Changed 4 years ago by marcin

  • Milestone changed from Kea-proposed to Kea1.0
  • Owner changed from UnAssigned to marcin

comment:5 follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

I reviewed commit c270a430d3e8b69507706519e0e806bb9cfcb25d

The change in the thread.cc is good and it does what it supposed to do. However, this change should be tested. I propose a short test which runs the thread and the thread checks and returns the value of the pthread_sigmask, executed from it.

The test verifies that all signals are blocked. A stub of such test could look like this:

void
getSignalMask(sigset_t* sigset) {
    pthread_sigmask(SIG_UNBLOCK, 0, sigset);
}

TEST(ThreadTest, sigmask) {
    sigset_t sigset;
    sigemptyset(&sigset);
    Thread thread(boost::bind(getSignalMask, &sigset));
    ASSERT_NO_THROW(thread.wait());
    EXPECT_EQ(1, sigismember(&sigset, SIGINT));
    EXPECT_EQ(1, sigismember(&sigset, SIGUSR1));
    EXPECT_EQ(1, sigismember(&sigset, SIGTERM));
}

I think it would be also good if you modify the SignalSet? in this ticket to use pthread_sigset, instead of sigprocmask.

comment:6 in reply to: ↑ 5 Changed 4 years ago by fdupont

Replying to marcin:

I reviewed commit c270a430d3e8b69507706519e0e806bb9cfcb25d

The change in the thread.cc is good and it does what it supposed to do. However, this change should be tested. I propose a short test which runs the thread and the thread checks and returns the value of the pthread_sigmask, executed from it.

=> I agree it should be a good addition (and thanks for the proposal).

I think it would be also good if you modify the SignalSet? in this ticket to use pthread_sigset, instead of sigprocmask.

=> already covered by #3060 (more generic changes now Kea is multi-threaded). BTW despite all docs pthread_sigmask() and sigprocmask() are equivalent so there is nothing really urgent to fix...

comment:7 Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin

Added the unit test and a comment in signal_set.h explaining if the signal code is not thread safe the thread code is now signal safe...

comment:8 Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

Reviewed commit 2695f920d5a94dd7e5705395ecb9eeb34e85e827

I updated the thread test with a test description, so please pull my change. It is ready to go.

I think this also needs a changelog entry: "libutil: block signals in the non-root thread", or something similiar.

comment:9 Changed 4 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged. Closing.

comment:10 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.