Avoid log file descriptor leakage

[bunk@…]

When trac is configured to log to a file and sends mail notifications via sendmail, SELinux complains:

type=AVC msg=audit(1356034258.828:30164):
 avc:  denied  { append } for  pid=28546 comm="sendmail"
 path="/tmp/trac.log" dev=sda1 ino=673386
 scontext=system_u:system_r:system_mail_t:s0
 tcontext=unconfined_u:object_r:httpd_tmp_t:s0 tclass=file

Attached patch against the current trunk revision r11179 sets the FD_CLOEXEC flag on the log file descriptor to avoid leaking it to forked children including sendmail.

More info about fd leaks and SELinux: ​http://danwalsh.livejournal.com/53603.html

[Remy Blank]

Interesting, thanks for the patch. I have a few questions.

• Wouldn't the same be necessary for all open file descriptors, for example the database file for SQLite?
• Could we solve this in a more generic way by setting close_fds=True when using the subprocess module (on non-Windows only)?
• Why does sendmail try to write to a file descriptor it didn't open in the first place?

[anonymous]

Good questions.

• SQLite uses close on exec since 2007: ​https://www.sqlite.org/src/info/f1e5fed8eb
• Yes, close_fds is better and shorter for this case: ​http://docs.python.org/2/library/subprocess.html#popen-constructor And no: there might be other places where trac executes a subprocess.
• I suspect closing all fds till 99 causes the SELinux check: ​http://code.metager.de/source/xref/postfix-debian/src/sendmail/sendmail.c#950

[anonymous]

Ping (sorry to bug you, but this is an easy to fix issue, so if you don't have further questions just apply one or both ways to fix it)

[Peter Suter]

Replying to rblank:

• Could we solve this in a more generic way by setting close_fds=True when using the subprocess module (on non-Windows only)?

Looks like we already do that for all other calls to subprocess.call and subprocess.Popen:

• source:trunk/tracopt/versioncontrol/git/tests/PyGIT.py@11483:164#L164​
• source:trunk/tracopt/versioncontrol/git/PyGIT.py@11483:122-125#L120​
  • (could be simplified using trac.util.compat.close_fds)
• source:trunk/trac/tests/functional/testenv.py@11483:125-127,143-145,150-153,177-179,228-229#L125​
  • (imports both trac.util.compat.close_fds and trac.tests.functional.compat.close_fds)
  • (also contains one more Windows-only call call)
• source:trunk/trac/db/postgres_backend.py@11483:174-175,194#L174​
• source:trunk/trac/db/mysql_backend.py@11483:225#L225​
• source:trunk/trac/tests/functional/svntestenv.py@11483:20-21,23-24#L20​

Some history digging suggests this was an oversight, not a deliberate decision:

• [5951] Added close_fds=True parameters (in functional testing sandbox)
• [6001] Added close_fds=close_fds Windows compat flag
• [7935] Introduced SendmailEmailSender (in trunk)
• [8027] Moved flag to trac.util.compat
• [8031] Added some more usages (still in testing sandbox)
• [8131] Merged flag from testing sandbox to trunk

[Remy Blank]

Thanks for checking! So this is a strong indicator that the close_fds solution is the right one.

[Remy Blank]

Added close_fds when calling sendmail in [11496].

[anonymous]

Great! Thanks for fixing :)

[Peter Suter]

OT: Coincidentally today ​pep:0433 was posted, proposing some easier way to suppress file descriptor leaks. (For Python 3.4)