1576728 - Crash in oly64.dll (FYunZip)

Status: RESOLVED FIXED

(External Software Affecting Firefox :: Other, defect, P2)

Description

[[:philipp]]

This bug is for crash report bp-347969b3-b777-467c-8b9f-1786e0190826.

Top 10 frames of crashing thread:

0 oly64.dll oly64.dll@0xe43c 
1 ntdll.dll RtlProcessFlsData 
2 ntdll.dll LdrShutdownThread 
3 ntdll.dll RtlExitUserThread 
4 kernelbase.dll FreeLibraryAndExitThread 
5 ucrtbase.dll crt_at_quick_exit 
6 ucrtbase.dll o_strcat_s 
7  @0x3611012a 
8 mozglue.dll static void patched_BaseThreadInitThunk mozglue/build/WindowsDllBlocklist.cpp:617
9 ntdll.dll RtlUserThreadStart 

these crashes with involvement of the module oly64.dll/oly.dll are showing up increasingly since firefox 68. this seems to come from an unpacking tool "FYunZip" from the company "浙江自贸区耀光网络科技有限公司", so it's predominantly users from zh-CN builds that are affected.

some more information about the software: https://any.run/report/d94f55f0c15d14d1706205a043ef9515c9a11ecfde374a43953bf65a4b689c55/e273acf5-59bb-4aad-8d5d-c40498b53b72

i don't have attempted to reproduce this myself, however i find it strange that an unzipping tool would require hooking into the browser process. should we attempt to blocklist this dll preemptively?

Comment 1

[[:philipp]]

Attachment: bug1576728.patch

Comment 2

[yxu]

This module comes from a unzipping software called "风云压缩".
Official website: http://www.yaoguangsoft.cn/
download link: http://dl.yaoguangsoft.cn/zip/setup_FYunZip.exe
Also, I can't reproduce this crash. According to the online search, this software is easy to bundle in other software installations. It will pop-up ads on background. Recent user feedback did not mention this software.

Comment 3

[Marco Castelluccio [:marco]]

I think we can add it to the blocklist if we can verify that it works without adverse effects.

Comment 4

[[:philipp]]

i found more crashes that look in common caused by another similar chinese unzipping software ("puddingzip").

in addition i tested a local build with blocklisting in place and these third party tools installed in a vm and didn't notice any problems while browsing around for a bit. i could not verify that the block is effective though, since i have found no way of reproducing the crash and the offending dlls aren't showing up in the modules list of crash reports. i think the only way to verify if blockisting is effective is to monitor if crashes go away in 1-2 beta cycles unfortunately.

Comment 5

[[:philipp]]

Attachment: bug1576728.patch

Comment 6

[Marco Castelluccio [:marco]]

Comment on attachment 9093620 [details] [diff] [review]
bug1576728.patch

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

Testing the block in the real world sounds fine to me, but requesting review from aklotz too who might know of some adverse effect this could cause.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9093620 [details] [diff] [review]
bug1576728.patch

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

With the launcher process now in effect, we need to be more careful about these.

I would be willing to accept a version of this patch that restricts itself to Nightly, at least to start with.

::: mozglue/build/WindowsDllBlocklistDefs.in
@@ +220,5 @@
>      # 360 Safeguard/360 Total Security causes a11y crashes, bug 1536227.
>      DllBlocklistEntry("safemon64.dll", ALL_VERSIONS),
> +
> +    # FYunZip and PuddingZip cause crashes, bug 1576728
> +    DllBlocklistEntry("oly64.dll", ALL_VERSIONS),

If you add `condition='NIGHTLY_BUILD'` as the final argument to each of these entries, I would be willing to land this.

Comment 8

[[:philipp]]

Attachment: bug1576728.patch

thanks aaron.

what would be the criteria to eventually let this move on to other release channels as well - just if we don't notice those crashes anymore after a considerable time on nightly or are you rather looking for no reports of other kind of breakage from users with these 3rd party tools as a result of this blocklisting patch?

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to [:philipp] from comment #8)

what would be the criteria to eventually let this move on to other release channels as well - just if we don't notice those crashes anymore after a considerable time on nightly or are you rather looking for no reports of other kind of breakage from users with these 3rd party tools as a result of this blocklisting patch?

My biggest concern is that, with this patch applied, Firefox can't start at all. Not only would the crash rate drop to zero, but those users would cease to be Firefox users.

The ideal situation would be that specific affected user(s) can confirm a working Firefox with this patch applied. Or that somebody can actually QA this block by reproducing the crash without the patch, and then successfully starting a non-crashing Firefox with the patch applied.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9094617 [details] [diff] [review]
bug1576728.patch

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

Thanks

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ba05fadf202
Add crashing dlls from chinese unzipping tools to the blocklist. r=aklotz

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6ba05fadf202

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Not sure this should be marked as a regression. In any case, if there isn't immediate further work to do here then I'm removing it from regression triage.

Comment 15

[yxu]

I have sent some message to the users who suffer from this bug and I received one user reply about PuddingZip. He said there was no crash when using nightly version (with the patch). However, he didn't crash again when he using the previous version. Although it can't explain we fixed this, it should not cause firefox can't start problem in comment 9 that Aaron warried about.

Comment 16

[[:philipp]]

would this single piece of feedback be enough to let the block-listing ride the trains in your judgement?
so far i am not aware of any nightly issues that have sprung up as a result of this, but i obviously lack good insight into the chinese market or support requests that originate from there...

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Do we have a callstack for oly64.dll in the third-party modules ping?

Comment 18

[Toshihito Kikuchi [:toshi]]

There is no oly64.dll in the untrusted module ping, but we have two callstacks loading PDZipMenu64.dll, which also causes the crash at RtlProcessFlsData. One callstack is nsDragService::InvokeDragSessionImpl in the main thread, and the other is nsIconChannel::GetHIconFromFile in the ImageIO thread.

On Win10 FYunZip, I can see oly64.dll is loaded into firefox when I opened the common dialog because it's integrated with Explorer.

I think blocking oly64.dll and pdzipmenu64.dll will not break Firefox launch, but I have some concern from blocking all versions. Given that Win7 users are hitting this crash more often than Win10 users, and interestingly I cannot see any reports from the latest Win10 (= 10.0.18362), this crash may not happen on the latest platform.

Comment 19

[Toshihito Kikuchi [:toshi]]

When I googled the crashing function, I found this MSDN article which looks the same issue If so, this was the old VC compiler issue. It's possible that the latest product no longer causes this.

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Perhaps for now we should restrict the block to the latest version for which we have seen crash reports.

Comment 21

[[:philipp]]

unfortunately i see crashes for all windows versions (including the win10 1903 update)...

FYunZip: https://crash-stats.mozilla.com/search/?signature=~oly.dll&signature=~oly64.dll&product=Firefox&platform=Windows&date=%3E%3D2019-06-01&_facets=signature&_facets=version&_facets=platform_pretty_version&_facets=useragent_locale&_facets=release_channel&_facets=cpu_arch&_facets=process_type&_facets=platform_version#facet-platform_version
PuddingZip: https://crash-stats.mozilla.com/search/?signature=~pdzipmenu64.dll&signature=~pdzipmenu32.dll&product=Firefox&platform=Windows&date=%3E%3D2019-06-01&_facets=signature&_facets=version&_facets=platform_pretty_version&_facets=useragent_locale&_facets=release_channel&_facets=cpu_arch&_facets=process_type&_facets=platform_version#facet-platform_version

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

Actually, I was referring to the latest version of the infringing DLLs.

Comment 23

[Toshihito Kikuchi [:toshi]]

Oh, maybe I set a wrong date range to browse crash reports.

The challenge is when this crash happens, the module is already unloaded, whicn means the version info is not available in the crash ping.

The current version of oly/oly64.dll is 1.1.3.19920. Assuming this version is ok, we may start with blocking oly.dll older than 1.1.3.19920.

For PuddingZip, I cannot find the installer. According to the untrusted module ping, we have data from three clients, and none of the instances have version info. I think blocking with UNVERSIONED would be a good start.

Comment 24

[[:philipp]]

Attachment: bug1576728_version.patch

thanks for the clarification.

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Do you mind using phabricator for this?

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #25)

Do you mind using phabricator for this?

Comment 27

[[:philipp]]

not in principle, but i'm not set-up to do this yet and unfortunately have a few more important things on my plate currently. i will try to look into it in 1-2 weeks time...

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

Can you please import this into Phabricator and then send me the review?

Comment 29

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1576728 - Let blocklist entries for FYUnZIP and PuddingZip ride the trains but confine them to older/unversioned dlls. r=aklotz

Comment 30

[Toshihito Kikuchi [:toshi]]

(In reply to Aaron Klotz [:aklotz] from comment #28)

Can you please import this into Phabricator and then send me the review?

Done!

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9107505 [details] [diff] [review]
bug1576728_version.patch

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

Moved to phabricator.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3322c8c93b02
Let blocklist entries for FYUnZIP and PuddingZip ride the trains but confine them to older/unversioned dlls. r=aklotz

Comment 33

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3322c8c93b02

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:gcp, maybe it's time to close this bug?

Comment 35

[Gian-Carlo Pascutto [:gcp]]

I see we're still hitting this crash though, so perhaps Toshi can comment if there's still something to do.

Comment 36

[Toshihito Kikuchi [:toshi]]

In contrast to my previous comment, now that the ping is available from all builds, we have lots of loading reports of these modules.

Looking at several callstacks, all stacks indicate both modules are loaded via shell extension, expecially as the icon overlay handler, which means blocking the module is not so harm. I'll prepare a patch to block the modules up to the latest version we can observe.

Below is one of the examples loading oly64.dll.

0 mozglue!patched_LdrLoadDll(wchar_t*, unsigned long*, _UNICODE_STRING*, void**)+0x1f3
1 kernelbase!LoadLibraryExW+0x16f
2 combase!LoadLibraryWithLogging(LoadOrFreeWhy,unsigned short const *,unsigned long,HINSTANCE__ * *)+0x2c
3 combase!CClassCache::CDllPathEntry::LoadDll(DLL_INSTANTIATION_PROPERTIES &,long (*&)(_GUID const &,_GUID const &,void * *),long (*&)(HSTRING__ *,IActivationFactory * *),long (*&)(void),HINSTANCE__ * &)+0x55
4 combase!CClassCache::CDllPathEntry::Create(DLL_INSTANTIATION_PROPERTIES &,bool,CClassCache::CDllPathEntry * &)+0x4f
...
20 combase!CoCreateInstance+0xc3
21 shell32!_SHCoCreateInstance(_GUID const &,IUnknown *,unsigned long,int,EXTCOCREATEFLAGS,_GUID const &,void * *)+0x211
22 shell32!DCA_SHExtCoCreateInstance+0x3f
23 shell32!CFSIconOverlayManager::_s_LoadIconOverlayIdentifiers(CDSA<_GUID> const *,CDSA<FSIconOverlay> *)+0x166
24 shell32!CFSIconOverlayManager::LoadNonloadedOverlayIdentifiers()+0xa4
25 shell32!EnableExternalOverlayIdentifiers()+0x4b
26 shell32!CFSIconOverlayManager::RefreshOverlayImages(unsigned long)+0xe5
27 shell32!GetIconOverlayManagerImpl+0x80
28 Windows.Storage!CFSFolder::_GetOverlayInfo(_ITEMID_CHILD const *,int *,unsigned long)+0x101
29 Windows.Storage!CIconOverlayTask::InternalResumeRT()+0x274
30 Windows.Storage!CRunnableTask::Run()+0x126
...

Comment 37

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1576728 - Block more versions of oly[64].dll and pdzipmenu[32|64].dll. r=gcp

Since we learned these modules are a shell exntension, blocking in the browser
process should suffice.

Comment 38

[Toshihito Kikuchi [:toshi]]

Attachment: Windows_DLLBlocklist_request_form.txt

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b8199ac5ba9
Block more versions of oly[64].dll and pdzipmenu[32|64].dll.  r=gcp

Comment 40

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0b8199ac5ba9