Set up DLL blocklist before LoadAppInitDlls (Port Bug 932100 to Thunderbird)

RESOLVED FIXED in Thunderbird 31.0

Status

defect
RESOLVED FIXED
6 years ago
10 months ago

People

(Reporter: Irving, Assigned: standard8)

Tracking

({regression})

unspecified
Thunderbird 31.0
x86
Windows 7
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird29+ fixed, thunderbird30+ fixed)

Details

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #947599 +++

From: Bug 932100 - Set up DLL blocklist before LoadAppInitDlls

> The DLL blocklist is ineffective against AppInit DLLs because they get
> loaded before the DLL blocklist is initialized. Bug 925459 is an example.
> 
> kernel32!LoadAppInitDlls is called during user32's DLL_PROCESS_ATTACH. In
> order to intercept AppInit DLLs, we would need to hook LdrLoadDll before
> user32 gets loaded.
> 
> Currently, firefox.exe has a load-time link to user32.dll, which means we do
> LoadAppInitDlls before any Mozilla code is executed. It turns out that this
> link to user32 is only used by one call to MessageBoxW from an error handler
> in nsBrowserApp.cpp. Given the rarity of that codepath, we could easily turn
> it into a runtime dynamic link.
> 
> That would at least let us run firefox!wmain before user32, but the
> blocklist is implemented in xul.dll, and by the time we load that,
> InitXPCOMGlue has already loaded a ton of stuff, including user32.
> 
> Some potential options:
> 1) Have a separate blocklist in firefox.exe for AppInit DLLs
> 2) Move the blocklist from xul.dll to firefox.exe
> 3) Disentangle things and try to get xul!XRE_SetupDllBlocklist available
> before InitXPCOMGlue preloads the world
Please note that this change has caused many headaches in FF, due to unforeseen interactions with external applications. Bug 951827 tracks the ongoing attempts to mitigate. Technical details of the problem are in bug 951827 comment 15.
I've ported bug 762310, part of bug 755724, bug 932100, and pushed these to try server:

https://hg.mozilla.org/try-comm-central/rev/3a14b5636cd6

Hopefully the external application issues mentioned in comment 1 are now long resolved and we won't have too much of an issue.

(Picking up as this could have caused bug 990676).
Assignee: nobody → standard8
> Hopefully the external application issues mentioned in comment 1 are now
> long resolved and we won't have too much of an issue.

After several attempts, those issues were finally resolved by Attachment #8383957 [details] [diff]. You may want to pick up that as well.
Posted patch The fixSplinter Review
Try server builds ended up here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=77d5bce20e8b

Using the crashme extension (https://code.google.com/p/crashme/) the try server build crashes & produces the crash reporter and submits it to crash-stats.

When I had tried one build previously (I forget which, probably beta), Thunderbird just hung when hitting the crash me button.

This ports bug 762310, part of bug 755724, bug 932100 - not all of these are required, but they got out app file a bit closer to Firefox's in the areas we need them to be.
Attachment #8404634 - Flags: review?(irving)
Blocks: 990676
(In reply to David Major [:dmajor] (UTC+12) from comment #3)
> > Hopefully the external application issues mentioned in comment 1 are now
> > long resolved and we won't have too much of an issue.
> 
> After several attempts, those issues were finally resolved by Attachment
> #8383957 [details] [diff]. You may want to pick up that as well.

That's all toolkit/core so we've already picked those up.
Comment on attachment 8404634 [details] [diff] [review]
The fix

Review of attachment 8404634 [details] [diff] [review]:
-----------------------------------------------------------------

I really wish we had an automated test for this, but the FF bugs don't appear to have one...

::: mail/app/nsMailApp.cpp
@@ +165,5 @@
> +#ifdef DEBUG
> +  // In order to be effective against AppInit DLLs, the blocklist must be
> +  // initialized before user32.dll is loaded into the process (bug 932100).
> +  if (GetModuleHandleA("user32.dll")) {
> +    fprintf(stderr, "DLL blocklist was unable to intercept AppInit DLLs.\n");

This is a MOZ_ASSERT in the Firefox version; do we really want to downgrade to just a printf?
Attachment #8404634 - Flags: review?(irving) → review+
(In reply to :Irving Reid from comment #6)
> I really wish we had an automated test for this, but the FF bugs don't
> appear to have one...

I was thinking the same thing. 

I am certain we had (manual) tests in litmus. I am unsure if a crash test is on usul's release checklist and in the newer moztap.  ludo?
Flags: needinfo?(ludovic)
(In reply to :Irving Reid from comment #6)
> Comment on attachment 8404634 [details] [diff] [review]
> The fix
> 
> Review of attachment 8404634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really wish we had an automated test for this, but the FF bugs don't
> appear to have one...
> 
> ::: mail/app/nsMailApp.cpp
> @@ +165,5 @@
> > +#ifdef DEBUG
> > +  // In order to be effective against AppInit DLLs, the blocklist must be
> > +  // initialized before user32.dll is loaded into the process (bug 932100).
> > +  if (GetModuleHandleA("user32.dll")) {
> > +    fprintf(stderr, "DLL blocklist was unable to intercept AppInit DLLs.\n");
> 
> This is a MOZ_ASSERT in the Firefox version; do we really want to downgrade
> to just a printf?
Yes. See Bug 939043 - Downgrade the user32 assertion to a warning message
https://hg.mozilla.org/mozilla-central/rev/c149ae799213
As Philip says, the MOZ_ASSERT was changed in a later bug/commit, so we do want that.
https://hg.mozilla.org/comm-central/rev/7ef9725513af

Lets hope that fixes the crash one as well.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Comment on attachment 8404634 [details] [diff] [review]
The fix

[Triage Comment]
I've proven this fixes the crash issue on windows, so taking across to aurora & beta for more testing.
Attachment #8404634 - Flags: approval-comm-beta+
Attachment #8404634 - Flags: approval-comm-aurora+
Flags: needinfo?(ludovic)
See Also: → 1198497
Duplicate of this bug: 778683
You need to log in before you can comment on or make changes to this bug.