One of my local builds of Firefox was failing to start this morning, which was puzzling when I did a build in a different directory and it was fine. Note: I've marked this as a security-sensitive bug although I don't think it is - but to be on the safe side. The failure mode was: $ ./mach run -P looptest -purgecaches Couldn't load XPCOM. On debugging further, the changes introduced by bug 1048687 lead to finding that we're reading uninitialised memory on startup: aXPCOMFile is: /Users/mark/loop/geckodev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL const char *tempSlash = strrchr(aXPCOMFile, '/'); size_t tempLen = size_t(tempSlash - aXPCOMFile); ... char tempBuffer[MAXPATHLEN]; memcpy(tempBuffer, aXPCOMFile, tempLen); This section should create tempBuffer with: /Users/mark/loop/geckodev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS However, note that tempBuffer is uninitialised and there's no null-terminator due to the memcpy, so doing printf on my failing build, I see: /Users/mark/loop/geckodev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS =/x? Next we do: const char *slash = strrchr(tempBuffer, '/'); tempLen = size_t(slash - tempBuffer); This is meant to find the last slash in that buffer, which on my Firefox build ends up at index 74, it then does another copy from aXPCOMFile, and appends the Resources/dependentlist.list. I should end up with: /Users/mark/loop/geckodev/objdir-ff/dist/NightlyDebug.app/Contents/Resources/dependentlibs.list However, due to the uninitialized memory, the directory ends up with: /Users/mark/loop/geckodev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/X/Resources/dependentlibs.list and hence the build fails to run.

This fixes the issue for me, by adding a null terminator after the copy so that strrchr can work correctly. I can't see any other issues elsewhere.

Mark, the code you patched has been superceded by one of the patches for bug 1079655 (https://hg.mozilla.org/mozilla-central/rev/6b845af11ff0). But the bug you found is still there, even in the new patch.

> even in the new patch I mean the patch for bug 1079655, not your patch.

(In reply to Steven Michaud from comment #2) > Mark, the code you patched has been superceded by one of the patches for bug > 1079655 (https://hg.mozilla.org/mozilla-central/rev/6b845af11ff0). But the > bug you found is still there, even in the new patch. I don't see how that code supersedes the code I'm touching. The code in nsXPCOMGlue is still being called - I was testing with today's fx-team which already has the patches from that bug.

Oops, you're right. Sorry. The code you changed is superficially similar to the code changed by that patch for bug 1079655. I didn't look closely enough.

And there's nothing wrong with that patch for bug 1079655, either. CFURLGetFileSystemRepresentation() always null-terminates its results. Sigh.

Comment on attachment 8504028 [details] [diff] [review] Fix unable to start Firefox due to 'Couldn't load XPCOM'. Gah, we don't have strncpy or something reasonable here? My kingdom for a std::string :-(

I looked at strncpy, but that also doesn't create a null-terminator in this instance. https://hg.mozilla.org/integration/mozilla-inbound/rev/beae97bde5ca

Comment on attachment 8504028 [details] [diff] [review] Fix unable to start Firefox due to 'Couldn't load XPCOM'. Approval Request Comment [Feature/regressing bug #]: Bug 1048687 [User impact if declined]: On Mac, Firefox could occasionally fail to start up [Describe test coverage new/current, TBPL]: Landed on TBPL [Risks and why]: Low risk - simple addition to ensure a string is only read to the intended end of string. [String/UUID change made/needed]: None.

If an attacker can manipulate your startup directory to take advantage of this bug then they can do worse things.

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5c326a2f65f https://hg.mozilla.org/releases/mozilla-beta/rev/c212fd07fd32