Closed Bug 1159826 Opened 5 years ago Closed 5 years ago

ensure_copy_recursive() leaks directory streams

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

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

People

(Reporter: mcs, Assigned: rstrong)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(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) {
         break;
       }
     }
   }

+  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
Thanks!
Attachment #8599421 - Flags: review?(spohl.mozilla.bugs)
Attachment #8599421 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/d84644c09199
Flags: in-testsuite-
Target Milestone: --- → mozilla40
Comment on attachment 8599421 [details] [diff] [review]
patch

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?
https://hg.mozilla.org/mozilla-central/rev/d84644c09199
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8599421 [details] [diff] [review]
patch

[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.