Mac OS X leak in nsUpdateDriver::ApplyUpdate
Categories
(Toolkit :: Application Update, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: robert.strong.bugs, Assigned: spohl)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
5.44 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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;
}
![]() |
Reporter | |
Comment 1•5 years ago
|
||
Stephen, no rush but if you could fix this as time permits I'd appreciate it.
Assignee | ||
Comment 2•5 years ago
|
||
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 | ||
Updated•5 years ago
|
![]() |
Reporter | |
Comment 3•5 years ago
|
||
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.
![]() |
Reporter | |
Comment 4•5 years ago
|
||
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
Assignee | ||
Comment 5•5 years ago
|
||
![]() |
Reporter | |
Comment 6•5 years ago
|
||
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)
Assignee | ||
Comment 7•5 years ago
|
||
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.
![]() |
Reporter | |
Comment 8•5 years ago
|
||
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
Assignee | ||
Comment 9•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9677c67b09409735efdc4a63da14c1753d61d13 Bug 1523613: Fix memory leaks during updates. r=rstrong
Assignee | ||
Comment 10•5 years ago
|
||
(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•5 years ago
|
||
bugherder |
Comment 12•5 years ago
|
||
== 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
Description
•