Closed Bug 1159826 Opened 5 years ago Closed 5 years ago

ensure_copy_recursive() leaks directory streams


(Toolkit :: Application Update, defect)

Not set



Tracking Status
firefox37 --- wontfix
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- fixed


(Reporter: mcs, Assigned: rstrong)


(Blocks 1 open bug)


(Whiteboard: [tor])


(1 file)

We received a report that an update of Tor Browser failed with the message: "ensure_copy_recursive: path is not a directory: .../Cache/4/99, rv: 0, err: 24"

Errno 24 is EMFILE aka "Too many open files."  In reviewing the code, we found that ensure_copy_recursive() in updater.cpp is missing a call to closedir().

Something like this should fix the problem:

--- a/toolkit/mozapps/update/updater/updater.cpp
+++ b/toolkit/mozapps/update/updater/updater.cpp
@@ -797,16 +797,17 @@ static int ensure_copy_recursive(const NS_tchar *path, const NS_tchar *dest,
                    NS_T("%s/%s"), dest, entry->d_name);
       rv = ensure_copy_recursive(childPath, childPathDest, skiplist);
       if (rv) {

+  NS_tclosedir(dir);
   return rv;

 // Renames the specified file to the new file specified. If the destination file
 // exists it is removed.
 static int rename_file(const NS_tchar *spath, const NS_tchar *dpath,
                        bool allowDirs = false)
Attached patch patchSplinter Review
Attachment #8599421 - Flags: review?(spohl.mozilla.bugs)
Attachment #8599421 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed to mozilla-inbound
Flags: in-testsuite-
Target Milestone: --- → mozilla40
Comment on attachment 8599421 [details] [diff] [review]

Requesting now since I will be on pto soon

Approval Request Comment
[Feature/regressing bug #]: Bug 307181
[User impact if declined]: It is possible that some users may error during update with too many open files
[Describe test coverage new/current, TreeHerder]: landed on m-i and soon to be on m-c. Tested locally.
[Risks and why]: Minimal. This is an obvious correctness fix
[String/UUID change made/needed]: None
Attachment #8599421 - Flags: approval-mozilla-beta?
Attachment #8599421 - Flags: approval-mozilla-aurora?
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8599421 [details] [diff] [review]

[Triage Comment]
Taking it in 38.0RC1 (because 38 is an ESR).
Attachment #8599421 - Flags: approval-mozilla-release+
Attachment #8599421 - Flags: approval-mozilla-beta?
Attachment #8599421 - Flags: approval-mozilla-aurora?
Attachment #8599421 - Flags: approval-mozilla-aurora+
Robert, could you please provide some steps to verify this?
Flags: needinfo?(robert.strong.bugs)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #10)
> Robert, could you please provide some steps to verify this?

I mean verification on ESR 38 particularly.
This is a correctness fix that would take a significant effort to simulate a failure condition which is likely why we've never had a bug report for it previously. The patch landing, the tests passing, and updates being successful will likely need to be enough for verification.
Flags: needinfo?(robert.strong.bugs)
Got it, thank you Robert! No manual verification for this then.
Flags: qe-verify-
Blocks: meta_tor
You need to log in before you can comment on or make changes to this bug.