Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#4895 closed Bug (fixed)

SIGHUP race during session init

Reported by: gvdl Owned by: gvdl
Priority: Normal Milestone: 2.60
Component: Daemon Version: 2.51
Severity: Minor Keywords: commit-request patch-provided
Cc: gvdl@…

Description

During code inspection for another bug (#4329) I noticed that there is a race condition where the wrong configuration dir can be used if a SIGHUP arrives before tr_sessionInit() returns for the first time.

I have attached a patch that does a minimal change that addresses this problem, however, the signal handling is a bit downmarket and needs to be fixed. 'signal' is archaic and the 'sigaction(2)' group of routines should be used instead.

Attachments (3)

daemon-sighup-minimal.patch (4.1 KB) - added by gvdl 9 years ago.
Minimal daemon.c patch
daemon-sighup-minimal.2.patch (3.3 KB) - added by jordan 9 years ago.
4895-SIGHUP_Minimal.3.patch (3.3 KB) - added by gvdl 9 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 9 years ago by livings124

Hey gbdl, it doesn't look like your patch attached. Could you re-upload?

Changed 9 years ago by gvdl

Minimal daemon.c patch

comment:2 in reply to: ↑ 1 Changed 9 years ago by gvdl

Replying to livings124:

Hey gbdl, it doesn't look like your patch attached. Could you re-upload?

Done. Sorry for the delay I spent yesterday setting up my local mercurial repository for Transmission.

comment:3 Changed 9 years ago by gvdl

  • Status changed from new to assigned

comment:4 Changed 9 years ago by gvdl

  • Keywords commit-request added

Fixed awaiting commit

comment:5 Changed 9 years ago by gvdl

  • Cc gvdl@… added
  • Keywords patch-provided added

Changed 9 years ago by jordan

comment:6 Changed 9 years ago by jordan

So the big picture is I agree with your reading in this ticket and your patch is good. However I have a few minor changes in daemon-sighup-minimal.2.patch for your feedback:

  • Added an info message telling the user that we've deferred the reload
  • Removed the assertions since they don't appear to be necessary?
  • Kept the sig handler func's original structure so it still has only one return point
  • Trimmed the comment verbosity a bit

comment:7 Changed 9 years ago by jordan

  • Milestone changed from None Set to 2.53

Changed 9 years ago by gvdl

comment:8 Changed 9 years ago by gvdl

Ok, I agree with your changes. The long comments were for the committer rather than for the code base, so removing those are fine and I've removed another and added it as a comment to another bug I'm working on. I'm a bit uncomfortable with getting rid of all of the asserts, so I've split the signal handler default: case into to and now tr_err track any unexpected signals that make it there. I'm going to rebase another patch on top of this, can you look at #4162 and grab that in the 2.53 revision too. It is a small add-on to this change to this change.

Last edited 9 years ago by gvdl (previous) (diff)

comment:9 Changed 9 years ago by jordan

Looks good.

comment:10 Changed 9 years ago by jordan

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

Committed in r13312

comment:11 Changed 9 years ago by livings124

  • Milestone changed from 2.53 to 2.60
Note: See TracTickets for help on using tickets.