Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#1550 closed defect (fixed)

file descriptor leak in filestore.retrieve()

Reported by: sascha_silbe Owned by: sascha_silbe
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Git as of bugdate
Severity: Major Keywords: r+
Cc: Distribution/OS: Unspecified
Bug Status: Assigned

Description

filestore.retrieve() contains a file descriptor leak:

        attempt = 1
        while os.path.exists(destination_path):
            if attempt > 10:
                fd_, destination_path = tempfile.mkstemp(prefix=uid,
                                                         suffix=extension,
                                                         dir=destination_dir)
                del fd_
                os.unlink(destination_path)
                break
            else:
                file_name = '%s_%s%s' % (uid, attempt, extension)
                destination_path = os.path.join(destination_dir, file_name)
                attempt += 1

Since fd_ is a plain integer (i.e. not a file object), "del fd_" will not close the file descriptor, but leave it open.

What's the reason for having two different ways of choosing names, BTW? Why don't we just use a single mkstemp() call and be done with it?

Attachments (2)

sugar-datastore-bug-1550-minimal.patch (1.2 KB) - added by sascha_silbe 10 years ago.
Fix file descriptor leak in filestore.retrieve(), minimally invasive version
sugar-datastore-bug-1550-mkstemp-only.patch (1.7 KB) - added by sascha_silbe 10 years ago.
Fix file descriptor leak in filestore.retrieve() and use only mkstemp()

Download all attachments as: .zip

Change History (10)

Changed 10 years ago by sascha_silbe

Fix file descriptor leak in filestore.retrieve(), minimally invasive version

comment:1 Changed 10 years ago by sascha_silbe

  • Keywords r? added
  • Owner changed from tomeu to sascha_silbe
  • Status changed from new to accepted

I've attached two possible fixes, a minimally invasive one and another that removes the loop and relies entirely on mkstemp().
Personally I prefer the second one as it eliminates the possibility of triggering bugs on rare occasions (i.e. only if there are already more than ten checked-out copies of the entry lying around).

Changed 10 years ago by sascha_silbe

Fix file descriptor leak in filestore.retrieve() and use only mkstemp()

comment:2 Changed 10 years ago by sascha_silbe

  • Cc alsroot added

Please review.

comment:3 Changed 10 years ago by alsroot

  • Keywords r+ added; r? removed

not sure but just in case, could you push 1st patch to sucrose-0.84/6 branches and 2nd one to the master

and for 2nd patch, better to preserve previous naming scheme, it was "%s_%s%s" (just in case)

comment:4 Changed 10 years ago by tomeu

  • Cc alsroot removed
  • Component changed from sugar-datastore to sugar-artwork
  • Keywords r? added; r+ removed

This one is on risk of being lost in trac and never applied. Maybe we should have a report to detect reviewed patches that weren't pushed yet?

comment:5 Changed 10 years ago by tomeu

  • Component changed from sugar-artwork to sugar-datastore

ping

comment:7 Changed 7 years ago by dnarvaez

  • Component changed from sugar-datastore to Sugar

comment:8 Changed 7 years ago by dnarvaez

  • Milestone 0.88 deleted

Milestone 0.88 deleted

Note: See TracTickets for help on using tickets.