Opened 7 years ago

Closed 7 years ago

Last modified 5 years 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: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: sjmurdoch, ioerror Actual Points:
Parent ID: #1775 Points:
Reviewer: Sponsor:


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 7 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 7 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 7 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 7 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:4 Changed 7 years ago by nickm

Cc: sjmurdoch ioerror added

See branch "cloexec" in my public repo.

comment:5 Changed 7 years ago by nickm

Status: newneeds_review

comment:6 Changed 7 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 7 years ago by sjmurdoch

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

comment:7 Changed 7 years ago by nickm

I've merged that onto my branch; thanks!

comment:8 Changed 7 years ago by sjmurdoch

See also changeset 15f2b78 in my branch cloexec of 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 7 years ago by sjmurdoch

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

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

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?

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 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:11 Changed 5 years ago by nickm

Keywords: tor-relay added

comment:12 Changed 5 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.