safe_init_db leaks temporary file descriptor

Bug #370845 reported by Matt Nordhoff
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
loggerhead
Fix Released
Low
Matt Nordhoff

Bug Description

From loggerhead/changecache.py:

def safe_init_db(filename, init_sql):
    # To avoid races around creating the database, we create the db in
    # a temporary file and rename it into the ultimate location.
    fd, temp_path = tempfile.mkstemp(dir=os.path.dirname(filename))
    con = dbapi2.connect(temp_path)
    ...

The file descriptor returned by mkstemp() is completely ignored, and left open forever.

Steps to reproduce:

>>> from loggerhead.changecache import safe_init_db
>>> for i in xrange(100):
... safe_init_db('/tmp/test.sql', "create table RevisionData (revid "
... "binary primary key, data binary)")
...

Then check something like "lsof -n". Look, 100 open copies of /tmp/test.sql!

I assume the simplest fix would be throwing in an "os.close(fd)". It'd be even better if pysqlite had some "connect_to_fd" function, but I don't know if that's the case.

Related branches

Changed in loggerhead:
importance: Undecided → Low
status: New → Confirmed
Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

I added an os.close(fd) in revision 336 of the trunk. I hope you don't mind.

I don't see how this could introduce any race conditions that weren't already there, so it seems okay to me.

Changed in loggerhead:
assignee: nobody → Matt Nordhoff (mnordhoff)
status: Confirmed → Fix Committed
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Yikes, thanks for finding and fixing this :)

The basic issue has been around for a while, I think...

Martin Albisetti (beuno)
Changed in loggerhead:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.