startup crash in xptiInterfaceEntry::GetConstantCount with randomly hex-code named DLL

RESOLVED FIXED in Firefox 39

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kairo, Assigned: dmajor)

Tracking

({crash})

unspecified
Firefox 42
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39+ fixed, firefox38.0.5 unaffected, firefox40+ fixed, firefox41+ fixed, firefox42+ fixed)

Details

(crash signature)

Attachments

(5 attachments, 2 obsolete attachments)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-1c6bf243-1768-41be-a6ff-792ee2150405.
=============================================================

This startup crash is the largest startup crash in 39 Dev Edition right now, with >5% of all crashes.

Those crashes all have randomly-named DLLs on the stack like 09c3ffd6f1a34fde86e1.dll or cc70bb3b2ef142de8fdb.dll in addition to ones with the longer name that starts in the same hey code, like 09c3ffd6f1a34fde86e1d448e8559c21.dll or cc70bb3b2ef142de8fdb656b2089cabf.dll

This sounds a lot like malware. The signature so far is only hitting 39 and 40, it started on nightly with the 3/28 build, apparently.
David, any idea if we can do something there?
Flags: needinfo?(dmajor)
|count| is null at xptiInterfaceInfo::GetConstantCount. 

Our blocklist won't be useful here. At best we could treat the symptoms with a null check. Who knows if the app will just crash somewhere else.
Flags: needinfo?(dmajor)
Posted patch bandaidSplinter Review
Nathan are you the right reviewer for this file?

I admit this is a dodgy fix. I don't have any better ideas though. Suggestions welcome.
Attachment #8588658 - Flags: review?(nfroyd)
Yuck.  One wonders if the malware authors even test their malware...

It looks like this is:

http://www.herdprotect.com/09c3ffd6f1a34fde86e1.dll-01293ce891e46edd81bb2baa0640f7e3a863f528.aspx

"Part of the Yontoo adware component, a web browser plugin that injects unwanted ads in the browser. The module 09c3ffd6f1a34fde86e1.dll by Girafarri has been detected as adware by 33 anti-malware scanners. It will plug into the web browser and display context-based advertisements by overwriting existing ads or by inserting new ones on various web pages."

Instead of this check, could we extend our blocklist check for hex dlls here:

http://mxr.mozilla.org/mozilla-central/source/mozglue/build/WindowsDllBlocklist.cpp#571

to handle DLLs that are only composed of hex characters?
Flags: needinfo?(dmajor)
Comment on attachment 8588658 [details] [diff] [review]
bandaid

Canceling review while we discuss a better solution.
Attachment #8588658 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Instead of this check, could we extend our blocklist check for hex dlls here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mozglue/build/
> WindowsDllBlocklist.cpp#571
> 
> to handle DLLs that are only composed of hex characters?

Last time we did such a thing, bsmedberg was pretty concerned about false positives. Flagging for an opinion.
Flags: needinfo?(dmajor) → needinfo?(benjamin)
Non-malware folks are actually using DLLs with 80-bit+ hex names?  Ugh.
I was in particular worried about <hex{8}>.* names accidentally conflicting. I'm relatively fine with blocking <hex{16,20}>.*
Flags: needinfo?(benjamin)
Tracking for 39+ since this is a topcrash.
I'm not happy about the direction we're headed in with these heuristics. We can't hope to be an antimalware engine. I'll do this patch as recommended, but I hope we'll be able to deal with these with self-support in the future.
Attachment #8588658 - Attachment is obsolete: true
Attachment #8589317 - Flags: review?(benjamin)
Comment on attachment 8589317 [details] [diff] [review]
block long hex names

I'm guessing an automated test for this would be magically difficult?
Attachment #8589317 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5250df3b304

FWIW I tested a bunch of edge cases locally.
https://hg.mozilla.org/mozilla-central/rev/c5250df3b304
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8589317 [details] [diff] [review]
block long hex names

Approval Request Comment
[Feature/regressing bug #]: external application
[User impact if declined]: startup crashes
[Describe test coverage new/current, TreeHerder]: green on m-i, local testing
[Risks and why]: Main risk is if a legitimate binary matches this hex pattern. We looked into this for a previous bug and there weren't any.
[String/UUID change made/needed]: none
Attachment #8589317 - Flags: approval-mozilla-aurora?
Comment on attachment 8589317 [details] [diff] [review]
block long hex names

Approved for uplift to aurora. Sounds like this is not ideal but is the best option for now to prevent a high volume startup crash caused by malware.
Attachment #8589317 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
this signature is returning in large amounts in 39.0b7
This is the #2 topcrash for 39.0b7. Re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is still a 96% startup crash in 39.0b7, and it's #1 with 50% of all crashes in that version. This is *really* bad right now.
David is there any chance you can look into this?
Flags: needinfo?(dmajor)
Nathan since you worked on this bug earlier is this something you could help with now? Thanks.
Flags: needinfo?(nfroyd)
Assuming I'm looking at the right place, it looks like this is coming from different DLLs than it was before, so these DLLs aren't getting blocked.  Maybe they wised up to our blocking and they renamed their DLLs?

I can't get the correlations to load, so I can't get a sense of how many crashes are due to which DLLs.

This is looking like the next step in the arms race. :(
Flags: needinfo?(nfroyd)
Presumably this is the result of the nsIInterfaceInfo.idl change in https://hg.mozilla.org/mozilla-central/rev/1652874e2d97 ?  Should we consider changing that in some way, e.g., by moving the new method to the end?
Yeah, formerly it was hex, now it's random-ASCII: Xmnjnyfx.dll

What's weird is I can't figure out why they'd be working around the block when it's just going to crash.

Changing the ordering of the IDL from bug 997325 might resolve this, but also might cause problems with other addons. It would be pretty risk.

If this is a topcrash, why is it that we can't get an actual sample of this malware DLL? That would make it a lot easier to debug/figure out mitigation strategies.
Comment 25 is private: false
In many reports this file appears in a directory called "Pay-By-Ads\Yahoo! Search" but at other times it shows up in directories like "btclient" or "dlclient" so it's probably a common library.

We may want to try talking to the Pay-By-Ads developers to see if they develop this library or could tell us where it came from. (I don't have high hopes though)
My original proposal from attachment 8588658 [details] [diff] [review] still stands, but there's no telling whether we'll just crash somewhere else soon afterwards.
Flags: needinfo?(dmajor)
That might still be an improvement over a startup crash.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> What's weird is I can't figure out why they'd be working around the block
> when it's just going to crash.

Maybe it's new malware?  comment 28 makes it sound like a different addon than what it was before.

> Changing the ordering of the IDL from bug 997325 might resolve this, but
> also might cause problems with other addons. It would be pretty risk.

Seems pretty plausible, though; the method we'd be calling without bug 997325 applied would be getMethodInfo...wonder if the second arg register is a valid pointer in these crashes.  Why do you say that moving the new isMainProcessScriptableOnly() method would be pretty risky?
Flags: needinfo?(benjamin)
We could just take David's patch, build an rc2 and publish it to see what happens...
As Liz said, that is better than nothing.
Yes, I like that idea. David, let's try landing your original attachment if you think it might lead us to more information or mitigating the startup crash somewhat. 

Nathan, my thought is that if it were new malware, we would be seeing this crash also spike on earlier betas.  But we aren't. It is only showing up in significant numbers for beta 7. 

According to this,
https://crash-analysis.mozilla.com/rkaiser/2015-06-22/2015-06-22.buildcrashes.html#Firefox-beta
ADI for beta 6 is at over 500K and beta 7 is at 1.3M. 
From crash-stats, we have 111 crashes total for beta 6 and 66201 for beta 7.
   
That's why it seems to me that it is more likely than not to be a regression from the uplifts between beta 6 and 7:  http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=cf82a529f8e9&tochange=900c78b9be48
Flags: needinfo?(nfroyd)
Flags: needinfo?(dmajor)
None of those uplifts look to be related to the crashes, though.  The things on the stack are all related to DOM-y/docshell-y stuff; very few of those uplifts touch anything in DOM or docshell, and the things that touch DOM don't look like they some anywhere near the stuff on the stack.

I would be OK with applying/uplifting David's original patch, but at this late stage, I guess we're not really sure whether things wouldn't crash someplace else...should have gone with the simpler patch in the first place.  My mistake. :(
Flags: needinfo?(nfroyd)
Attachment #8588658 - Attachment is obsolete: false
Flags: needinfo?(dmajor)
Attachment #8588658 - Flags: review?(nfroyd)
I'm happy to give the null-check a try. What's the backup plan if it doesn't work?
Comment on attachment 8588658 [details] [diff] [review]
bandaid

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

I have no ideas for a backup plan.  I can't imagine what the addon will be expected by passing in a null pointer...
Attachment #8588658 - Flags: review?(nfroyd) → review+
Comment on attachment 8588658 [details] [diff] [review]
bandaid

Approval Request Comment
[Feature/regressing bug #]: external binary
[User impact if declined]: high-volume startup crashes
[Describe test coverage new/current, TreeHerder]: zero
[Risks and why]: Our main risk at this point is that this patch won't be enough to stop the crashes
[String/UUID change made/needed]: none
Attachment #8588658 - Flags: approval-mozilla-beta?
Attachment #8588658 - Flags: approval-mozilla-aurora?
Comment on attachment 8588658 [details] [diff] [review]
bandaid

Taking it in aurora for testing (even if I don't have much hope here).
and updating the flag.
Attachment #8588658 - Flags: approval-mozilla-release?
Attachment #8588658 - Flags: approval-mozilla-beta?
Attachment #8588658 - Flags: approval-mozilla-aurora?
Attachment #8588658 - Flags: approval-mozilla-aurora+
On the theory that they were actually attempting to call GetMethodInfo before the vtable change, I kinda worry that their code might look like:

for(i = 0, ...) {
  foo->GetMethodInfo(i, &ptr);
  ...
}

In which case the null check only helps us on the first iteration, and we'd crash with count==1 on the next iteration. Maybe we could do a try/catch around the write to the pointer? I also worry that they might not check the return value and use an uninitialized "ptr" parameter.

I'd be curious to hear Benjamin's specific concerns about reverting the vtable order. Yeah, it sucks to change a vtable and UUID this late, but we're in a pretty bad spot here.
Comment on attachment 8588658 [details] [diff] [review]
bandaid

Let's take it to release to be able to have treeherder results.
Attachment #8588658 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-release/rev/f075b15b50d5
Assignee: nobody → dmajor
Target Milestone: Firefox 40 → ---
David, is there anything Manual QA can do here to help? My understanding is that this crash affects only setups that contain specific DLLs so it'll be pretty tough to reproduce the crash on our end without a sample. Either way, if you think there's something we should be looking at here, please let me know.
Flags: needinfo?(dmajor)
https://hg.mozilla.org/mozilla-central/rev/98a929f58730
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(In reply to Andrei Vaida, QA [:avaida] from comment #45)
It would be very helpful if you could obtain a sample of the "Pay-By-Ads" addon. I don't know where to find it though. Maybe it comes bundled with some freeware?
Flags: needinfo?(dmajor)
> On the theory that they were actually attempting to call GetMethodInfo
> before the vtable change, I kinda worry that their code might look like:
> 
> for(i = 0, ...) {
>   foo->GetMethodInfo(i, &ptr);
>   ...
> }
> 
> In which case the null check only helps us on the first iteration, and we'd
> crash with count==1 on the next iteration.

And that's exactly what happened.
https://crash-stats.mozilla.com/report/index/d618976c-d905-42ba-8356-ec1642150625
https://crash-stats.mozilla.com/report/index/288ffaee-ac55-40bf-b32f-c7f732150625
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Check callersSplinter Review
Attachment #8626415 - Flags: review?(nfroyd)
Comment on attachment 8626415 [details] [diff] [review]
Check callers

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

Yuck.  But what else are you going to do?

I still think it might be worth flipping the IDL and trying that first.  Flipping the IDL would fix the topcrash, guaranteed, but open us up to some unknown (but we think it's likely to be fairly small) number of crashes.  This is going to stop things from crashing, but we don't know if it's going to cause problems later on down the line.

This is one of those times that you wish you could release to a small population of users at a time...

::: xpcom/reflect/xptinfo/xptiprivate.h
@@ +412,5 @@
>  private:
>      xptiInterfaceEntry* mEntry;
>      nsRefPtr<xptiInterfaceInfo> mParent;
> +#ifdef XP_WIN
> +    static void* BrokenModule;

I would really like it if this was called sBrokenModule.
Attachment #8626415 - Flags: review?(nfroyd) → review+
Once more with feeling, let's land this on m-r.
Flags: needinfo?(ryanvm)
Flags: needinfo?(dmajor)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a894452f3e9

For the record, my preference is to try this patch first (vs reverting the IDL), because addons who are doing the right thing with the 39 SDK will not notice a difference. I worry that there are a nontrivial number of such addons, who would be broken if we revert the IDL.

The risk of the current approach is that it depends on the broken addon safely handling our errors. If the addons uses its (possibly-uninitialized) parameters despite our error codes, the crashes may continue. If that happens we can revert and try more desperate options.
Flags: needinfo?(benjamin)
(In reply to David Major [:dmajor] from comment #56)
> It didn't work :-( The crashes just moved elsewhere:
> 
> https://crash-stats.mozilla.com/search/
> ?platform=Windows+NT&product=Firefox&version=39.
> 0b&build_id=20150626112833&_facets=signature&_facets=build_id&_facets=version
> &_facets=release_channel&_columns=date&_columns=signature&_columns=product&_c
> olumns=version&_columns=build_id&_columns=platform#crash-reports
> 
> We'll need to back out and look at other options.

Stupid question time: the crashes from this query ("signature facet" tab) look much more spread out and somewhat lower volume (?) than the startup crash.  For instance, the OOM signature looks like it already has several bugs associated with it, and I don't see why it would be correlated with these changes.  What motivates attributing the crashes from that link to the changes from this bug?
Flags: needinfo?(dmajor)
> Stupid question time: the crashes from this query ("signature facet" tab)
> look much more spread out and somewhat lower volume (?) than the startup
> crash.  For instance, the OOM signature looks like it already has several
> bugs associated with it, and I don't see why it would be correlated with
> these changes.  What motivates attributing the crashes from that link to the
> changes from this bug?

It's more spread out because every machine now has a unique signature due to the random filenames, and there's a long tail. In total these crashes are in a similar ballpark as the xpti signature before, and it's still a startup crash.
Flags: needinfo?(dmajor)
I'm still really scared of reverting the vtable order. But I'll cave if others really want to try it.

Could we live with this if it was a one-time startup crash rather than a permanent startup crash? What if, upon the first null pointer, we write the caller's filename into some temp file and block that on subsequent launches?

Any other ideas?
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/a1089f3645fc
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
(In reply to David Major [:dmajor] from comment #59)
> Could we live with this if it was a one-time startup crash rather than a
> permanent startup crash?

I think we could. When Firefox crashes at a startup, I'm pretty sure the vast majority of people will try again. If it succeeds then, I think we're good.
Writing out the module name seems pretty reasonable.  Questions:

- Where is this file going to be stored?
- What happens if multiple places call into this with a null pointer?  Do we block all of them?
- Probably there are other things I've not thought of yet.

I still kind of want to revert the vtable change.
Flags: needinfo?(nfroyd)
> - Where is this file going to be stored?
> - What happens if multiple places call into this with a null pointer?  Do we
> block all of them?
> - Probably there are other things I've not thought of yet.

The blocklist is initialized before we have a profile, so we can't use the profile directory. I was originally thinking ::GetTempPath but that's kind of yucky. Perhaps we could use the crash reporter's pre-profile area (Users\...\Firefox\Crash Reports).

For now I'm envisioning an extremely targeted fix with just a single entry in the file, overwriting any previous versions of the file. At some point we may want to make it more general, but not for 39.
(In reply to David Major [:dmajor] from comment #63)
> For now I'm envisioning an extremely targeted fix with just a single entry
> in the file, overwriting any previous versions of the file. At some point we
> may want to make it more general, but not for 39.

I fully agree with that. I've been of the opinion for a while that we should have the full DLL blocklist in an external file that could potentially be updated without a full Firefox update (of course we'd still need a mechanism for that which does not run inside the Firefox process). But let's take that to a followup.
So, current status:

- dmajor is currently en route home from #mozwww.  He has volunteered to write the patch described in comment 59.  This patch is probably pretty safe, unless there are *other* misbehaving binary addons that we're not aware of...?  I am also a little leery of giving outside programs the ability to block arbitrary DLLs in Firefox, but maybe that is not really a problem...

- I have written a patch that fixes the vtable ordering issues.  This patch would at least fix the crash that we're seeing, but might cause problems for *other* binary addons that did update to the new vtable order promised in beta1.  Or it might turn up other issues.  Not sure.

Decisions:

- We need to decide between them, or run testing on both of them, or...something else.
- We should probably backout dmajor's patch, since it didn't fix the problem, on all relevant branches:

https://hg.mozilla.org/mozilla-central/rev/a1089f3645fc
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4e2c643442d
https://hg.mozilla.org/releases/mozilla-release/rev/0b0822cabbb9

Backing out the patch on central and aurora can be done straightaway; I guess we would want to wait on the m-r backout until we have another fix to land with it?

Liz, what next steps do release drivers want to take here?
Flags: needinfo?(lhenry)
Oh, and given that the patch didn't work, this is not fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

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

::: xpcom/reflect/xptinfo/nsIInterfaceInfo.idl
@@ +28,1 @@
>  interface nsIInterfaceInfo : nsISupports

Should we also rev the UUID on anything that hands out an nsIInterfaceInfo?
Fx41: https://hg.mozilla.org/mozilla-central/rev/8758612313fa
Fx39: https://hg.mozilla.org/releases/mozilla-release/rev/a70e2969c711

Aurora/Beta are closed for the uplift right now. I'll do the Fx40 backout from Beta after the uplifts are done. Leaving the status flag set to fixed as a reminder.
Target Milestone: Firefox 41 → ---
Trying to see if I can be of any help here...

In beta 7 I uplifted a patch that conditionally launches the Windows 10 modern Settings application [1]. IApplicationActivationManager is available starting with Windows 8, but I'm not experienced enough with crash-stats to find a way to see a breakdown of crashes per Windows version.

Perhaps this has something to do with the crashes? Note that this patch is also present on mozilla-aurora and mozilla-central, and the crashes appear to be specific to beta only as far as I can tell. Most likely not the cause, but if people are looking for ideas of things to investigate...

[1] http://hg.mozilla.org/releases/mozilla-beta/rev/5e03a12dd57e#l3.32
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #70)
The crashing machines have roughly our typical distribution of Windows versions. (If anything they have slightly more Windows 7 than usual, but not enough to make me suspicious)

From the symptoms that we're seeing, I'm convinced that the root cause is the vtable change in bug 997325. The spike in 39b7 is definitely strange, but at this point I'm not convinced that there is a causation.
Here's the patch to record the crashing module name, if we want it.

I'll be the first to admit that there are some yucky aspects of this patch:
- There's a lot of buffer manipulation
- I had to stick with GetTempFileW to get the temp path. It would have been nicer to use the crash reporter's directory, but right now mozglue.dll doesn't know about that location, and I think it's too risky to plumb it through at this point.
- Normally we don't hardcode the word Firefox, but if we're going to stick a strange file into a user's %TEMP%, I'd like to be clear on where it's coming from to avoid surprise.

If we opt for this approach I'd like to get several reviewers, including those familiar with Windows APIs and the crash reporter.
Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

I'm hoping at least two people can review this. Please look extra carefully at the buffer usage and Windows API calls.
Attachment #8627352 - Flags: review?(ted)
Attachment #8627352 - Flags: review?(benjamin)
Attachment #8627352 - Flags: review?(aklotz)
Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +3402,5 @@
> +    return;
> +
> +  // Don't lock ourselves out by mistake
> +  if (wcsstr(moduleName, L"xul.dll"))
> +    return;

Is xul.dll the only file to be whitelisted?
How about *adding* the old interface with the old iid?
dmajor mentions mozglue.... could the change in bug 868814 have anything to do with the crash?
Flags: needinfo?(lhenry) → needinfo?(mh+mozilla)
I don't see mozglue being mentioned as a cause of the problem. Merely as part of the technical details for the fix.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

General style nit: Curly braces around single line consequents of if statements

This looks good. My main concern is what :emk had to say about the whitelist. See my comment about dependentlibs.list. I'd either like to see another revision of this patch, or else convince me that my suggestion is unnecessary. :-)

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +600,5 @@
>  
> +  if (sPreviouslyCrashedModule[0] &&
> +      moduleFileName->Length < ArrayLength(sPreviouslyCrashedModule) &&
> +      !wcsicmp(sPreviouslyCrashedModule, moduleFileName->Buffer)) {
> +    printf_stderr("LdrLoadDll: Blocking load of previously crashed module '%s'\n", dllName);

Is this printf_stderr something you want to leave in? (I suppose I'm ok with it, just checking)

@@ +737,5 @@
> +                &nBytes,
> +                nullptr)) {
> +    sPreviouslyCrashedModule[0] = L'\0';
> +  }
> +

In the successful case:
sPreviouslyCrashedModule[nBytes] = L'\0';
since the saved filename does not include null terminator and to be consistent with the fact that you set it in the failed case.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +3402,5 @@
> +    return;
> +
> +  // Don't lock ourselves out by mistake
> +  if (wcsstr(moduleName, L"xul.dll"))
> +    return;

Ditto... it seems to me that the moduleName should not intersect with the set of libraries in dependentlibs.list
Attachment #8627352 - Flags: review?(aklotz) → review-
Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +204,5 @@
>  static DWORD sThreadLoadingXPCOMModule;
>  static bool sBlocklistInitFailed;
>  static bool sUser32BeforeBlocklist;
>  
> +static wchar_t sPreviouslyCrashedModule[MAX_PATH];

Should this be MAX_PATH+1 for the null?

@@ +715,5 @@
> +  wchar_t tempPath[MAX_PATH];
> +  if (!GetTempPathW(ArrayLength(tempPath), tempPath))
> +    return;
> +
> +  if (wcscat_s(tempPath, ArrayLength(tempPath), L"FirefoxBlockedDLL.txt"))

Why ArrayLength() here and earlier but sizeof() later?

@@ +735,5 @@
> +                sPreviouslyCrashedModule,
> +                sizeof(sPreviouslyCrashedModule),
> +                &nBytes,
> +                nullptr)) {
> +    sPreviouslyCrashedModule[0] = L'\0';

There are a couple places this function can bail out without initializing sPreviouslyCrashedModule. Maybe move this to the top of the function and do it unconditionally.

Do we always null-terminate the contents we write to the file? What if someone malicious (such as the dlls we're trying to block) puts something really long in there or otherwise not null terminated? I guess it wouldn't be terrible -- the wcsicmp() should stop at the end of the module name and that will be less than the length of the array.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +3402,5 @@
> +    return;
> +
> +  // Don't lock ourselves out by mistake
> +  if (wcsstr(moduleName, L"xul.dll"))
> +    return;

xul.dll isn't the only potential problem. I hope this is a short-lived hack because this does raise the possibility of mysterious failures to launch that will be pretty hard to diagnose, and which won't be fixed by reinstalling Firefox because we'll use the same temp file.
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #80)
> In the successful case:
> sPreviouslyCrashedModule[nBytes] = L'\0';

but watch out for nBytes == sizeof(sPreviouslyCrashedModule)
(In reply to Masatoshi Kimura [:emk] from comment #77)
> How about *adding* the old interface with the old iid?

Nathan and I discussed this last week. We think that the addon receives an nsIInterfaceInfo as an out-param, and uses it blindly without a QI, something like:
  nsIInterfaceInfo* ptr;
  foo->GetInterfaceInfo(&ptr);
  for (i = 0; ...)
    ptr->GetMethodInfo(i, ...); // goes to GetConstantCount

So responding to the old UUID wouldn't help there.
> Is xul.dll the only file to be whitelisted?
 
> This looks good. My main concern is what :emk had to say about the
> whitelist. See my comment about dependentlibs.list. I'd either like to see
> another revision of this patch, or else convince me that my suggestion is
> unnecessary. :-)

It's intended more as a fail-safe than a whitelist. No code other than the broken addon should ever be passing a null pointer, so we should never see an internal binary in this codepath. I just want to protect against "_ReturnAddress()" somehow being incorrect and blaming libxul further up (for example we might lose the real return address in a tail call, though I admit it's extremely unlikely in this situation). Do you think the other dependentlibs are at risk of such things?
I modified the search based on the moved crashes (comment 52), and remarked that the same address appeared in multiple of these libraries.

The following link seems to highlight some of the crashes with the same stack:

https://crash-stats.mozilla.com/search/?platform=Windows+NT&product=Firefox&signature=~%400x1ad64&_facets=signature&_facets=build_id&_facets=version&_facets=release_channel&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Looking at the top 3, of these libraries:
https://www.herdprotect.com/xmnjnyfx.dll-b7383216e66711b6d3d951158ffe23a976c1d2f2.aspx
https://www.herdprotect.com/hhhlmpyl.dll-40dc2f3ebe59cab2f42837d63055d95670f7ee5c.aspx
https://www.herdprotect.com/ubpouego.dll-cd0d2b436e0b1170d3bed11ce4150b3b684eefd2.aspx

we can notice that they have identical PE Metadata.
Maybe, this would be enough to filter out these libraries.

On the other hand, these are not the same as the one reported in comment 4.
(In reply to Nicolas B. Pierron [:nbp] from comment #85)

> we can notice that they have identical PE Metadata.
> Maybe, this would be enough to filter out these libraries.
> 
> On the other hand, these are not the same as the one reported in comment 4.

Indeed. Many of the modules have that same June timestamp, but I've also seen other variants. And it leaves us at risk of them shipping an update that still uses the old SDK.
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #80)
> General style nit: Curly braces around single line consequents of if
> statements

Will do.

> This looks good. My main concern is what :emk had to say about the
> whitelist. See my comment about dependentlibs.list. I'd either like to see
> another revision of this patch, or else convince me that my suggestion is
> unnecessary. :-)

Per comment 84 I'm still mostly worried about libxul, but if it makes you feel better, what if we delete the temp file if we reach the "Couldn't load XPCOM" error path?

> Is this printf_stderr something you want to leave in? (I suppose I'm ok with
> it, just checking)

printf_stderr is pretty common in that function. And I'm kind of hoping to get the vendor's attention that they're crashing.
 
> In the successful case:
> sPreviouslyCrashedModule[nBytes] = L'\0';
> since the saved filename does not include null terminator and to be
> consistent with the fact that you set it in the failed case.

Ok (modulo comment 82)
(In reply to Daniel Veditz [:dveditz] from comment #81)
> > +static wchar_t sPreviouslyCrashedModule[MAX_PATH];
> 
> Should this be MAX_PATH+1 for the null?

It wouldn't hurt anything.

> > +  if (wcscat_s(tempPath, ArrayLength(tempPath), L"FirefoxBlockedDLL.txt"))
> 
> Why ArrayLength() here and earlier but sizeof() later?

wcscat_s takes a number of wchar_t elements. WriteFile knows nothing about types so it speaks in bytes. Actually I could probably use the templated variant of wcscat to auto-infer the length.

> @@ +735,5 @@
> > +                sPreviouslyCrashedModule,
> > +                sizeof(sPreviouslyCrashedModule),
> > +                &nBytes,
> > +                nullptr)) {
> > +    sPreviouslyCrashedModule[0] = L'\0';
> 
> There are a couple places this function can bail out without initializing
> sPreviouslyCrashedModule. Maybe move this to the top of the function and do
> it unconditionally.

As a global it ought to be zeroed already, but I'm not opposed to adding an initialization anyway.

> Do we always null-terminate the contents we write to the file? What if
> someone malicious (such as the dlls we're trying to block) puts something
> really long in there or otherwise not null terminated? I guess it wouldn't
> be terrible -- the wcsicmp() should stop at the end of the module name and
> that will be less than the length of the array.

Indeed; that was my reason for adding the moduleFilename->Length check.
We performed exploratory testing around the latest versions of antivirus-related add-ons across Windows 7 64-bit on:
  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nfroyd@mozilla.com-de83156d7de9/try-win32/
  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dmajor@mozilla.com-568f47238738/try-win32/

For now, we are concerned about the following result:
- Norton Toolbar 2015.2.1.5 is automatically disabled despite the fact that xpinstall.signatures.required pref is set to false (This happened before Norton account was created) → http://i.imgur.com/ErXild7.png
- After Norton account was created and the install process was finished, we restart the Firefox -> An Error Message appeared: Couldn’t load XPCOM → http://i.imgur.com/TxbOnN7.jpg
- We tried to open Firefox from application, but another error message was displayed: Could not find the Mozilla runtime → http://i.imgur.com/sbA7hAT.jpg  

This behavior is not reproducible on Firefox 39 RC build 5 because the Norton Toolbar 2015.2.1.5 is not compatible with Firefox 39 → http://i.imgur.com/vC4oFaq.jpg

We have also installed several popular add-ons from addons.mozilla.org and no new issues were encountered. 

We will investigate further this issue. Our testing results are tracked in https://etherpad.mozilla.org/bug1151506
Thank you Vasilica! These are important findings. Did the errors appear with both of the try-builds, or just one?
Flags: needinfo?(vasilica.mihasca)
(In reply to David Major [:dmajor] from comment #90)
> Thank you Vasilica! These are important findings. Did the errors appear with
> both of the try-builds, or just one?

The errors are visible for both try-builds.
Flags: needinfo?(vasilica.mihasca)
(In reply to David Major [:dmajor] from comment #84)
> > Is xul.dll the only file to be whitelisted?
>  
> > This looks good. My main concern is what :emk had to say about the
> > whitelist. See my comment about dependentlibs.list. I'd either like to see
> > another revision of this patch, or else convince me that my suggestion is
> > unnecessary. :-)
> 
> It's intended more as a fail-safe than a whitelist. No code other than the
> broken addon should ever be passing a null pointer, so we should never see
> an internal binary in this codepath. I just want to protect against
> "_ReturnAddress()" somehow being incorrect and blaming libxul further up
> (for example we might lose the real return address in a tail call, though I
> admit it's extremely unlikely in this situation). Do you think the other
> dependentlibs are at risk of such things?

I see what you're saying. You're right, this is very unlikely to begin with. I think that it is completely possible for those other libs to be somewhere on the call stack, though you'd see libxul above the others. OTOH this could probably get really nutty if we start thinking of the full graph of dependent libraries. We need to draw a line somewhere.
> The errors are visible for both try-builds.

Hmm, that's strange. Do these errors happen on a regular Nightly?
Flags: needinfo?(alexandra.lucinet)
- Removed xul.dll check in favor of DeleteFile
- Added extra (perhaps excessive) safety on sPreviouslyCrashedModule
- Added curly braces
- I decided to stick with wcscat_s because the size-templated wcscat does not check for sufficient space in the buffer
Attachment #8627352 - Attachment is obsolete: true
Attachment #8627352 - Flags: review?(ted)
Attachment #8627352 - Flags: review?(benjamin)
Attachment #8627773 - Flags: review?(dveditz)
Attachment #8627773 - Flags: review?(aklotz)
Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

LGTM

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +712,5 @@
>  
> +void ReadPreviouslyCrashedModule()
> +{
> +  memset(sPreviouslyCrashedModule, 0, sizeof(sPreviouslyCrashedModule));
> +  

Nit: clear out the spaces from this blank line

::: xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp
@@ +778,5 @@
> +    }
> +
> +    if (!aConstantCount) {
> +#if defined(XP_WIN) && defined(MOZ_CRASHREPORTER)
> +        CrashReporter::RecordCrashingModule(_ReturnAddress());

Does this need to include intrin.h or anything for _ReturnAddress()? Meh, I guess as long as it builds...
Attachment #8627773 - Flags: review?(aklotz) → review+
Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

It's unfortunate that this file has to be in the tempdir. With additional plumbing we could probably store this in AppData/Roaming/Mozilla/Firefox, but that's probably too complex for the timeframe at hand.

Is the intention to remove this code in a cycle or two? I bet we could do better in a new release by de-XPCOMing all of the interfaceinfo stuff.
Attachment #8627773 - Flags: feedback+
Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +598,5 @@
>      }
>    }
>  
> +  if (sPreviouslyCrashedModule[0] &&
> +      moduleFileName->Length < ArrayLength(sPreviouslyCrashedModule) &&

Per docs (and verified in debugger) moduleFileName->Length is measured in bytes. I need to use sizeof here.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #96)
> It's unfortunate that this file has to be in the tempdir. With additional
> plumbing we could probably store this in AppData/Roaming/Mozilla/Firefox,
> but that's probably too complex for the timeframe at hand.

Yeah, getting that right is a risk that I don't want to take this late.
 
> Is the intention to remove this code in a cycle or two? I bet we could do
> better in a new release by de-XPCOMing all of the interfaceinfo stuff.

I would love to rip this out as soon as we can get away with it. I'll file a follow-up bug if this patch sticks.
(Aaron, Dmajor,

User Advocacy is preparing an infection control compaign to contact affected users.  We need guidance on what data you need / want from those users, and how to get it.  Happy to vidyo, email, or chat.

)
Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

Looks good except for the null-termination math. Normally I'd "r+ with nits" but since we're in a stressful last-minute situation it will be clearer for everyone (especially sheriffs doing branch transplants) to have a fresh patch.

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +737,5 @@
> +
> +  DWORD nBytes = 0;
> +  if (ReadFile(inputFile,
> +               sPreviouslyCrashedModule,
> +               sizeof(sPreviouslyCrashedModule) - 1,

minus 2 or sizeof(wchar_t) ?  If you round down when null terminating (nBytes/2 below) it's not terrible: we might stomp a byte we read, but it'd be an illegal half character anyway.

@@ +740,5 @@
> +               sPreviouslyCrashedModule,
> +               sizeof(sPreviouslyCrashedModule) - 1,
> +               &nBytes,
> +               nullptr)) {
> +    sPreviouslyCrashedModule[nBytes] = L'\0';

nBytes is bytes but the array is wchar_t -- this could be stomping way out in space and in any case won't be where we want it. On the other hand as long as the file name fills less than half the array the overly paranoid memset() initialization will make sure it's correctly null terminated.

You could sanity check the data -- if it's odd we didn't write it
   if (nBytes % 2)  <recover>

Definitely need to put the null at nBytes/2 though.
Attachment #8627773 - Flags: review?(dveditz) → review-
Blocks: 1178936
Carrying r=aklotz, and got r=dveditz by IRC. Thanks for catching that.

Approval Request Comment
[Feature/regressing bug #]: external app and bug 997325
[User impact if declined]: high-volume startup crashes
[Describe test coverage new/current, TreeHerder]: local testing under debugger
[Risks and why]: As before, we have the risk that this doesn't work. Also it's rather inelegant that we have to touch the temp directory. We'll especially need to watch how AV software reacts to this code.
[String/UUID change made/needed]: none
Attachment #8627773 - Attachment is obsolete: true
Attachment #8627853 - Flags: review+
Attachment #8627853 - Flags: approval-mozilla-release?
Flags: needinfo?(ted)
Comment on attachment 8627853 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

Approved to land on m-r.
Attachment #8627853 - Flags: approval-mozilla-release? → approval-mozilla-release+
This is building now and I would like to ship it to the beta channel in the morning. 

Florin, Vasilica, Alexandra (and anyone else who cares to test), it will probably be good to give this new build a try on Windows with antivirus software installed. I don't think we should test much more than that, but anything you can do will surely be helpful.
Flags: needinfo?(florin.mezei)
For anyone else who wants to test this, you will be able to download build 6 from this directory: ftp://ftp.mozilla.org/pub/firefox/candidates/39.0-candidates/    I believe we especially want testing on Windows.
(In reply to David Major [:dmajor] from comment #93)
> > The errors are visible for both try-builds.
> 
> Hmm, that's strange. Do these errors happen on a regular Nightly?

No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot: http://i.imgur.com/ErXild7.png), with or without an Norton account. With 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible -> http://i.imgur.com/IKwlq0o.png
Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could not find the Mozilla runtime" error messages are *only* applicable for the try-builds.

Please note that this testing is on-going and we will update here as soon as we are done.
Flags: needinfo?(alexandra.lucinet)
Testing was performed today with 39 RC build 6, covering Add-ons (AntiVirus and other more popular add-ons) and Web Compatibility (major websites with AV add-ons enabled). 

Testing went fine with no issues related to this fix.

Testing covered: Win 7 x64 (2 machine), Win 8.1 x86, Mac OS X 10.10.4, Ubuntu 14.04 x86. More testing details and complete results can be found at: https://etherpad.mozilla.org/Fx39RC6.
Flags: needinfo?(florin.mezei)
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #106)
> (In reply to David Major [:dmajor] from comment #93)
> > > The errors are visible for both try-builds.
> > 
> > Hmm, that's strange. Do these errors happen on a regular Nightly?
> 
> No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact
> that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot:
> http://i.imgur.com/ErXild7.png), with or without an Norton account. With
> 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7
> 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible ->
> http://i.imgur.com/IKwlq0o.png
> Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could
> not find the Mozilla runtime" error messages are *only* applicable for the
> try-builds.
> 
> Please note that this testing is on-going and we will update here as soon as
> we are done.

FF39-compatible extension for Norton Toolbar comes only with our 22.5+ versions of v22 products. (also released for our v21 and v20 products).

Here is a download link to Norton Security trial for the 22.5 version you should try in place of the current N360 trial link in your exploratory testing:

http://buy-download.norton.com/downloads/2015/22.5/NS/US/NS-ESD-22.5.0.124-EN.exe

Live Update must be run after install to apply a patch for FF39-compatible extension.
(In reply to Matt Powers from comment #108)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #106)
> > (In reply to David Major [:dmajor] from comment #93)
> > > > The errors are visible for both try-builds.
> > > 
> > > Hmm, that's strange. Do these errors happen on a regular Nightly?
> > 
> > No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact
> > that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot:
> > http://i.imgur.com/ErXild7.png), with or without an Norton account. With
> > 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7
> > 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible ->
> > http://i.imgur.com/IKwlq0o.png
> > Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could
> > not find the Mozilla runtime" error messages are *only* applicable for the
> > try-builds.
> > 
> > Please note that this testing is on-going and we will update here as soon as
> > we are done.
> 
> FF39-compatible extension for Norton Toolbar comes only with our 22.5+
> versions of v22 products. (also released for our v21 and v20 products).
> 
> Here is a download link to Norton Security trial for the 22.5 version you
> should try in place of the current N360 trial link in your exploratory
> testing:
> 
> http://buy-download.norton.com/downloads/2015/22.5/NS/US/NS-ESD-22.5.0.124-
> EN.exe
> 
> Live Update must be run after install to apply a patch for FF39-compatible
> extension.

My apologies, the download link above does not provide a trial product, but rather one that must be immediately activated with a product key (to allow access to the UI to let you run LiveUpdate).

I will find an alternate URL for a real trial-download.
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

I think we should do this for 40/41/42 (and land it in time for 40 beta 1!).  If Benjamin is around it would be nice to know if he agrees with that plan.
Attachment #8627232 - Flags: review+
Attachment #8627232 - Flags: feedback?(benjamin)
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Per discussion we'd like to land this on beta before 40b1 ships. It would allow us to keep the temp-file bandaid confined to version 39. If beta1 ships without this, then we won't be able to change the UUID until 41.

Approval Request Comment
[Feature/regressing bug #]: external app and bug 997325
[User impact if declined]: We'll have to keep the v39 hack in v40
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: This is low risk as long as it goes out with the first beta. Otherwise we'll have to wait another cycle.
[String/UUID change made/needed]: Yes /!\
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

See comment 111
Attachment #8627232 - Flags: approval-mozilla-beta?
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Approving for uplift to Beta as this is a startup crash that affects a significant set of end-users.
Attachment #8627232 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
OK. This also means we risk breaking some antivirus addons.  Since this puts things back to the vtable load order from March.

We don't know that it breaks anything, but we should test and react to that in early beta.
We expect that the vendors who are doing the right thing will recompile with the new SDK at beta 1 like always. Those who aren't, presumably didn't recompile for 39, so we'll be putting them back to what they're used to.
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

https://hg.mozilla.org/releases/mozilla-beta/rev/0e0e1e0151cb
(In reply to Matt Powers from comment #109)
> (In reply to Matt Powers from comment #108)
> > (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #106)
> > > (In reply to David Major [:dmajor] from comment #93)
> > > > > The errors are visible for both try-builds.
> > > > 
> > > > Hmm, that's strange. Do these errors happen on a regular Nightly?
> > > 
> > > No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact
> > > that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot:
> > > http://i.imgur.com/ErXild7.png), with or without an Norton account. With
> > > 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7
> > > 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible ->
> > > http://i.imgur.com/IKwlq0o.png
> > > Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could
> > > not find the Mozilla runtime" error messages are *only* applicable for the
> > > try-builds.
> > > 
> > > Please note that this testing is on-going and we will update here as soon as
> > > we are done.
> > 
> > FF39-compatible extension for Norton Toolbar comes only with our 22.5+
> > versions of v22 products. (also released for our v21 and v20 products).
> > 
> > Here is a download link to Norton Security trial for the 22.5 version you
> > should try in place of the current N360 trial link in your exploratory
> > testing:
> > 
> > http://buy-download.norton.com/downloads/2015/22.5/NS/US/NS-ESD-22.5.0.124-
> > EN.exe
> > 
> > Live Update must be run after install to apply a patch for FF39-compatible
> > extension.
> 
> My apologies, the download link above does not provide a trial product, but
> rather one that must be immediately activated with a product key (to allow
> access to the UI to let you run LiveUpdate).
> 
> I will find an alternate URL for a real trial-download.

Here is a working trialware download URL:

http://us.norton.com/norton-security-downloads-trial?lg=en&ct=US

You will need to create or supply an existing Norton Account.  

Then run LiveUpdate (under the Security section of the main UI) to apply patches - one of which is FF39 compatible Norton Toolbar.

Then start FF39 and if needed enable the extension.
(In reply to Liz Henry (:lizzard) from comment #114)
> OK. This also means we risk breaking some antivirus addons.  Since this puts
> things back to the vtable load order from March.

Does it matter? I heard Firefox 40+ dropped the support for binary XPCOM components in addons.
I found an installer for the offending adware on VirusTotal. Does anyone have a Windows VM handy to test it?
(In reply to Masatoshi Kimura [:emk] from comment #118)
> Does it matter? I heard Firefox 40+ dropped the support for binary XPCOM
> components in addons.

It'll probably be pushed back to 41, per bug 1179902.
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #119)
> I found an installer for the offending adware on VirusTotal. Does anyone
> have a Windows VM handy to test it?

Vladan, do you still need help testing this? After a discussion with our IT people it seems we could safely test this on a VM here, so if you still need help you can send me the installer and we'll give it a try.
Flags: needinfo?(vdjeric)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #121)
> Vladan, do you still need help testing this? After a discussion with our IT
> people it seems we could safely test this on a VM here, so if you still need
> help you can send me the installer and we'll give it a try.

Aaron Klotz tried it on a VM, I tried it without a VM. Sadly, launching the installer EXE was not enough to get the malware to infect the bait machines.

You can give it another shot if you like -- I'll forward you the EXE.
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #122)
> Aaron Klotz tried it on a VM, I tried it without a VM. Sadly, launching the
> installer EXE was not enough to get the malware to infect the bait machines.
> 
> You can give it another shot if you like -- I'll forward you the EXE.

I gave it a try yesterday and today, but same as you guys I also had no luck reproducing the crash.
Since this fix seems to be happy on beta, dmajor asked me to push it to inbound.  Will be nomming for Aurora next.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d278252858a
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Approval Request Comment
[Feature/regressing bug #]: bug 997325
[User impact if declined]: Crashes from malware binary addons/DLLs
[Describe test coverage new/current, TreeHerder]: The same change was made to our initial beta releases and seems to have worked out OK.
[Risks and why]: Low risk.
[String/UUID change made/needed]: One UUID change to nsIInterfaceInfo.  Shouldn't be a problem for Aurora.
Attachment #8627232 - Flags: feedback?(benjamin) → approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5d278252858a
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Already has been on Beta and m-c, closing the loop on Aurora.
Attachment #8627232 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.