Closed Bug 1081906 Opened 10 years ago Closed 10 years ago

Attempting to start Firefox can result in "Couldn't load XPCOM" due to reading uninitialised memory

Categories

(Core :: XPCOM, defect)

33 Branch
All
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 --- unaffected
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Attachment #8504028 - Flags: review?(benjamin)
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 :-(
Attachment #8504028 - Flags: review?(benjamin) → review+
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
Assignee: nobody → standard8
Target Milestone: --- → mozilla36
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.
Attachment #8504028 - Flags: approval-mozilla-beta?
Attachment #8504028 - Flags: approval-mozilla-aurora?
If an attacker can manipulate your startup directory to take advantage of this bug then they can do worse things.
Group: core-security
Attachment #8504028 - Flags: approval-mozilla-beta?
Attachment #8504028 - Flags: approval-mozilla-beta+
Attachment #8504028 - Flags: approval-mozilla-aurora?
Attachment #8504028 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.