1523613 - Mac OS X leak in nsUpdateDriver::ApplyUpdate

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Robert Strong (they/them - no direct email)]

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;
}

Comment 1

[Robert Strong (they/them - no direct email)]

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

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Patch

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!

Comment 3

[Robert Strong (they/them - no direct email)]

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 4

[Robert Strong (they/them - no direct email)]

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

>From: Stephen A Pohl <[e-mail]>
>
>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

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 6

[Robert Strong (they/them - no direct email)]

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)

Comment 7

[Stephen A Pohl [:spohl]]

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 8

[Robert Strong (they/them - no direct email)]

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

Comment 9

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9677c67b09409735efdc4a63da14c1753d61d13
Bug 1523613: Fix memory leaks during updates. r=rstrong

Comment 10

[Stephen A Pohl [:spohl]]

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

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e9677c67b094

Comment 12

[Florin Strugariu [:Bebe]]

== 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