#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)
Change History (10)
Changed 14 years ago by sascha_silbe
comment:1 Changed 14 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 14 years ago by sascha_silbe
Fix file descriptor leak in filestore.retrieve() and use only mkstemp()
comment:3 Changed 14 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 14 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 14 years ago by tomeu
- Component changed from sugar-artwork to sugar-datastore
ping
comment:6 Changed 14 years ago by alsroot
- Keywords r+ added; r? removed
- Resolution set to fixed
- Status changed from accepted to closed
sucrose-0.84
http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/71f000a02583edd0cac5a59b3228204c49f8412a
sucrose-0.86
http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/6e01fcbbb01a4d4f412286f98f18c19172977a49
master
http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/21c8e9c079bbe32a00a298dceecb551a3fbb1b56
http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/ba0a546d53bb10aa8540f9725488c8f7d0dde250
comment:7 Changed 10 years ago by dnarvaez
- Component changed from sugar-datastore to Sugar
Fix file descriptor leak in filestore.retrieve(), minimally invasive version