[tor-bugs] #31594 [Core Tor/Tor]: Close all the log fds before aborting

[Tor Bug Tracker & Wiki]

#31594: Close all the log fds before aborting
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.4.2.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  diagnostics, 042-should, android,    |  Actual Points:  0.5
  macos, 035-backport, 040-backport,             |
  041-backport                                   |
Parent ID:  #31571                               |         Points:  0.3
 Reviewer:  nickm                                |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by nickm):

 * status:  needs_review => needs_revision


Comment:

 I've left a couple of comments on the review. I've not reviewed the fsync
 commit, and I haven't checked that the new list of levels on the
 subsystems matches their dependency order or their order in
 subsystem_list.c.

 Here are my current thoughts on your questions, but for all of these
 cases, I'll defer to your judgment.

 > when we clear the list of error fds, should we use -1 or 0 as the
 placeholder value?

 If the n_sigsafe_log_fds value is zero, it should not matter what the
 empty entries contain.

 That said, -1 is more commonly used in our code for "not a valid FD."
 (''That'' said, we already use 0 here, and it might be better to leave
 that unchanged in this branch.)

 > are any of these bugs serious? Do they need a backport?

 IMO they don't currently warrant a backport, but they might warrant a
 backport some day.  They strike me as the kind of issue that we might
 change our mind about and really wish we had backported at some point in
 the future.  On the other hand, they also strike me as subtle enough to
 warrant extensive testing before we think of a backport.

 > should I split this PR up into multiple PRs?

 I don't think so, unless you want to. Maybe. (At first I thought that if
 we are considering a backport, we might want to backport only part of this
 branch.  But on the other hand, if we backport only part of this branch,
 we risk backporting something unstable that has not had testing.)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31594#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online