Closed Bug 1523613 Opened 6 years ago Closed 5 years ago

Mac OS X leak in nsUpdateDriver::ApplyUpdate

Categories

(Toolkit :: Application Update, defect, P1)

59 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: spohl)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Direct leak of 288 byte(s) in 6 object(s) allocated from:
#0 0x10d1fe233 in wrap_malloc (/usr/local/opt/llvm/lib/clang/7.0.1/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:x86_64+0x59233)
#1 0x10d143fad in moz_xmalloc (<objdir>/dist/Nightly.app/Contents/MacOS/libmozglue.dylib:x86_64+0x1fad)
#2 0x11ebb1451 in ApplyUpdate(nsIFile*, nsIFile*, nsIFile*, int, char**, bool, bool, int*) (<objdir>/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xd27b451)
#3 0x11ebafaa8 in ProcessUpdates(nsIFile*, nsIFile*, nsIFile*, int, char**, char const*, bool, int*) (<objdir>/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xd279aa8)
#4 0x11ebb4349 in nsUpdateProcessor::StartStagedUpdate() (<objdir>/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xd27e349)
#5 0x11ebc5827 in mozilla::detail::RunnableMethodImpl<nsUpdateProcessor*, void (nsUpdateProcessor::)(), true, (mozilla::RunnableKind)0>::Run() (<objdir>/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xd28f827)
#6 0x111bf40b3 in nsThread::ProcessNextEvent(bool, bool
) (<objdir>/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2be0b3)
#7 0x111bfa7dd in NS_ProcessNextEvent(nsIThread*, bool) (<objdir>/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c47dd)

Associated code

char **argv = new char *[argc + 1];
<snip>
#elif defined(XP_MACOSX)
UpdateDriverSetupMacCommandLine(argc, argv, restart);

UpdateDriverSetupMacCommandLine calls SetupMacCommandLine

and

void SetupMacCommandLine(int& argc, char**& argv, bool forRestart) {
sArgs = static_cast<char**>(malloc(kArgsGrowSize * sizeof(char*)));
<snip>
argc = sArgsUsed;
argv = sArgs;
}

Stephen, no rush but if you could fix this as time permits I'd appreciate it.

Flags: needinfo?(spohl.mozilla.bugs)
Attached patch Patch (obsolete) — Splinter Review

This changes SetupMacCommandLine to release the passed-in argv. This meant that we had to change to malloc/free everywhere that this function is called.

XRE_mainRun had to allocate a temporary copy of gArgv in one case, since we're now releasing it inside SetupMacCommandLine. This is not an issue with the two other calls to SetupMacCommandLine in nsAppRunner.cpp since it's using gRestartArgv, which was malloc'd.

I don't have time to test this out on Oak at the moment. Robert, would you mind taking this for a spin during the review? I wanted to post the patch as soon as possible, even if I didn't have time to test it out right now. Thank you!

Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9040603 - Flags: review?(robert.strong.bugs)
Priority: -- → P1

Thanks Stephen! I had to clobber my LSAN build due to invalid symlinks in the .app. I'll comment in the bug after I test it out and will land it on oak to verify basic functionality around this code.

Comment on attachment 9040603 [details] [diff] [review]
Patch

>From: Stephen A Pohl <spohl.mozilla.bugs@gmail.com>
>
>Bug 1523613: Fix memory leaks during updates. r=rstrong
>
>diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp
>--- a/toolkit/xre/nsUpdateDriver.cpp
>+++ b/toolkit/xre/nsUpdateDriver.cpp
>@@ -630,48 +630,50 @@ static void ApplyUpdate(nsIFile *greDir,
> 
> #if defined(XP_UNIX) && !defined(XP_MACOSX)
>   // We use execv to spawn the updater process on all UNIX systems except Mac
>   // OSX since it is known to cause problems on the Mac.  Windows has execv, but
>   // it is a faked implementation that doesn't really replace the current
>   // process. Instead it spawns a new process, so we gain nothing from using
>   // execv on Windows.
>   if (restart) {
>+    free(argv);
>     exit(execv(updaterPath.get(), argv));
use after free. Just remove free(argv);

>   }
>   *outpid = fork();
>   if (*outpid == -1) {
>-    delete[] argv;
>+    free(argv);
>     return;
>   } else if (*outpid == 0) {
>+    free(argv);
>     exit(execv(updaterPath.get(), argv));
same here

>   }
>-  delete[] argv;
The Linux block of code above has recently changed to just
  if (restart) {
    exit(execv(updaterPath.get(), argv));
  } else {
    *outpid = PR_CreateProcess(updaterPath.get(), argv, nullptr, nullptr);
  }
  delete[] argv;
With these changes you can just remove the delete[] argv; and let the free(argv); at the end handle it
Attached patch PatchSplinter Review
Attachment #9040603 - Attachment is obsolete: true
Attachment #9040603 - Flags: review?(robert.strong.bugs)
Attachment #9040641 - Flags: review?(robert.strong.bugs)

This fixes the leak in ApplyUpdate. The following now shows up in the log

rstrong/moz/_1_mozilla-central/ff-lsan-leak/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xe1e2077)
1:13.91 pid:59970 Direct leak of 823 byte(s) in 5 object(s) allocated from:
1:13.91 pid:59970 #0 0x10219b28c in wrap_strdup (/usr/local/opt/llvm/lib/clang/7.0.1/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5328c)
1:13.91 pid:59970 #1 0x11231791f in CommandLineServiceMac::SetupMacCommandLine(int&, char**&, bool) (/Users/rstrong/moz/_1_mozilla-central/ff-lsan-leak/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xdf5291f)
1:13.91 pid:59970 #2 0x112311872 in mozilla::detail::RunnableFunction<UpdateDriverSetupMacCommandLine(int&, char**&, bool)::$_0>::Run() (/Users/rstrong/moz/_1_mozilla-central/ff-lsan-leak/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xdf4c872)
1:13.91 pid:59970 #3 0x1046c64d9 in nsThread::ProcessNextEvent(bool, bool*) (/Users/rstrong/moz/_1_mozilla-central/ff-lsan-leak/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x3014d9)

Okay, that looks like it's coming from AddToCommandLine, and more specifically from the call to strdup of which the result is never free'd. This will require a bit of a rewrite to fix. Will take a look.

Comment on attachment 9040641 [details] [diff] [review]
Patch

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp
>--- a/toolkit/xre/nsAppRunner.cpp
>+++ b/toolkit/xre/nsAppRunner.cpp
>diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp
>--- a/toolkit/xre/nsUpdateDriver.cpp
>+++ b/toolkit/xre/nsUpdateDriver.cpp
>@@ -590,17 +590,17 @@ static void ApplyUpdate(nsIFile *greDir,
> 
>   int argc = 5;
>   if (restart) {
>     argc = appArgc + 6;
>     if (gRestartedByOS) {
>       argc += 1;
>     }
>   }
>-  char **argv = new char *[argc + 1];
>+  char** argv = static_cast<char**>(malloc((argc + 1) * sizeof(char*)));
>   if (!argv) {
>     return;
>   }
>   argv[0] = (char *)updaterPath.get();
>   argv[1] = (char *)updateDirPath.get();
>   argv[2] = (char *)installDirPath.get();
>   argv[3] = (char *)applyToDirPath.get();
>   argv[4] = (char *)pid.get();
>@@ -630,43 +630,43 @@ static void ApplyUpdate(nsIFile *greDir,
> 
> #if defined(XP_UNIX) && !defined(XP_MACOSX)
>   // We use execv to spawn the updater process on all UNIX systems except Mac
>   // OSX since it is known to cause problems on the Mac.  Windows has execv, but
>   // it is a faked implementation that doesn't really replace the current
>   // process. Instead it spawns a new process, so we gain nothing from using
>   // execv on Windows.
>   if (restart) {
>-    exit(execv(updaterPath.get(), argv));
>+    int execResult = execv(updaterPath.get(), argv);
>+    free(argv);
>+    exit(execResult);
>   } else {
>     *outpid = PR_CreateProcess(updaterPath.get(), argv, nullptr, nullptr);
>   }
>-  delete[] argv;
> #elif defined(XP_WIN)
>   if (isStaged) {
>     // Launch the updater to replace the installation with the staged updated.
>     if (!WinLaunchChild(updaterPathW.get(), argc, argv)) {
>-      delete[] argv;
You need a free(argv) here, right?

>       return;
>     }
>   } else {
>     // Launch the updater to either stage or apply an update.
>     if (!WinLaunchChild(updaterPathW.get(), argc, argv, nullptr, outpid)) {
>-      delete[] argv;
Same here

>       return;
>     }
>   }
>-  delete[] argv;
> #elif defined(XP_MACOSX)
>   UpdateDriverSetupMacCommandLine(argc, argv, restart);
>   // We need to detect whether elevation is required for this update. This can
>   // occur when an admin user installs the application, but another admin
>   // user attempts to update (see bug 394984).
>   if (restart && !IsRecursivelyWritable(installDirPath.get())) {
>-    if (!LaunchElevatedUpdate(argc, argv, outpid)) {
>+    bool hasLaunched = LaunchElevatedUpdate(argc, argv, outpid);
>+    free(argv);
>+    if (!hasLaunched) {
>       LOG(("Failed to launch elevated update!"));
>       exit(1);
>     }
>     exit(0);
>   }
> 
>   if (isStaged) {
>     // Launch the updater to replace the installation with the staged updated.
>@@ -679,16 +679,17 @@ static void ApplyUpdate(nsIFile *greDir,
>   if (isStaged) {
>     // Launch the updater to replace the installation with the staged updated.
>     PR_CreateProcessDetached(updaterPath.get(), argv, nullptr, nullptr);
>   } else {
>     // Launch the updater to either stage or apply an update.
>     *outpid = PR_CreateProcess(updaterPath.get(), argv, nullptr, nullptr);
>   }
> #endif
>+  free(argv);
>   if (restart) {
>     exit(0);
>   }
> }

Please file a followup for the strdup leak
Attachment #9040641 - Flags: review?(robert.strong.bugs) → review+
Blocks: 1530374

(In reply to Robert Strong (Robert he/him) [:rstrong] (use needinfo to contact me) from comment #8)

Please file a followup for the strdup leak

Filed bug 1530374.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

== Change summary for alert #19641 (as of Mon, 25 Feb 2019 13:16:48 GMT) ==

Improvements:

19% Resident Memory windows7-32 pgo stylo 562,005,198.13 -> 455,057,513.54

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19641

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: