#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)
Change History (14)
comment:1 follow-up: ↓ 2 Changed 9 years ago by livings124
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: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.
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
Hey gbdl, it doesn't look like your patch attached. Could you re-upload?