Opened 4 years ago

Closed 3 years ago

Last modified 19 months ago

#2029 closed defect (fixed)

Set FD_CLOEXEC on all open file descriptors so that they are closed on exec()

Reported by: sjmurdoch Owned by: sjmurdoch
Priority: normal Milestone: Tor: 0.2.3.x-final
Component: Tor Version:
Keywords: tor-relay Cc: sjmurdoch, ioerror
Actual Points: Parent ID: #1775
Points:

Description

Tor currently does not set FD_CLOEXEC on files it opens, so any child processes (e.g. tor-fw-helper) inherit open files. This is bad, so currently we try to guess the largest open file handle and close everything up to it, but this is icky and probably not reliable.

We should call fcntl(fd, F_SETFD, FD_CLOEXEC) on any files we open to avoid needing to do this. This will require finding all the cases where Tor and its libraries create a file descriptor, so not just open but also pipe, dup/dup2, socket, etc...

Tor currently does not exec anything except tor-fw-helper, so this should not affect anything else.

Child Tickets

Attachments (1)

0001-Fix-compile-error-on-MacOS-X-and-other-platforms-wit.patch (747 bytes) - added by sjmurdoch 3 years ago.
Patch to fix compile error on MacOS X (and other platforms without O_CLOEXEC)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by Sebastian

Can we think of a situation where we might need to pass fds to children later? Otherwise sounds like a good plan

comment:2 Changed 4 years ago by nickm

If we need to pass fds across an exec, we should do so explicitly by clearing (or not setting) FD_CLOEXEC on them IMO. I can't think of a case where we want to do an exec and pass all the fds to the new program.

comment:3 Changed 3 years ago by nickm

  • Milestone set to Tor: 0.2.3.x-final

comment:4 Changed 3 years ago by nickm

  • Cc sjmurdoch ioerror added

See branch "cloexec" in my public repo.

comment:5 Changed 3 years ago by nickm

  • Status changed from new to needs_review

comment:6 Changed 3 years ago by sjmurdoch

Checking now, but there was a compile error on MacOS X (which appears not to have O_CLOEXEC). I have attached a patch.

Changed 3 years ago by sjmurdoch

Patch to fix compile error on MacOS X (and other platforms without O_CLOEXEC)

comment:7 Changed 3 years ago by nickm

I've merged that onto my branch; thanks!

comment:8 Changed 3 years ago by sjmurdoch

See also changeset 15f2b78 in my branch cloexec of git://git.torproject.org/sjm217/tor.git

This stops the CLOEXEC flag being set twice (once by socket() and once by fcntl()) in a similar way to how file descriptors created through open() are already handled. I don't know whether the different treatment of socket() was intentional, and don't think it causes any problem, but felt it neater to treat socket and open in the same way unless there is a good reason not to.

comment:9 Changed 3 years ago by sjmurdoch

I made some changes to my branch (cloexec of git://git.torproject.org/sjm217/tor.git) after experimenting on MacOS X by seeing which file descriptors tor-fw-helper has open using lsof.

786abbd
tor-fw-helper had an open filehandle to the Tor log file; this patch fixes this

367794c
tor-fw-helper had open filehandles to the DNS resolvers; this patch fixes this. Note that by using tor_open_socket() rather than plain socket(), the socket accounting code now knows about these sockets. Is this a problem?

a961521
While debugging the issue below, I noted that tor_socketpair() uses FD_CLOEXEC before it is checked. This probably isn't an issue, but we check elsewhere so I cleaned this up.

I hoped that tor-fw-helper now would only have file descriptors 0, 1, and 2 open, but there is one left: a unix domain socket to /var/run/mDNSResponder. I guess this is something created by the MacOS X DNS code, and I am not sure how to set CLOEXEC on this file descriptor.

comment:10 Changed 3 years ago by nickm

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

Seems okay for now. I'll merge the code and close this ticket.

comment:11 Changed 19 months ago by nickm

  • Keywords tor-relay added

comment:12 Changed 19 months ago by nickm

  • Component changed from Tor Relay to Tor
Note: See TracTickets for help on using tickets.