Closed Bug 1576728 Opened 1 year ago Closed 5 months ago

Crash in oly64.dll (FYunZip)

Categories

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

All
Windows
defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox67 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 wontfix, firefox76 wontfix, firefox77 wontfix, firefox78 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: philipp, Assigned: philipp)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 3 obsolete files)

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?

Flags: needinfo?(mcastelluccio)
Attached patch bug1576728.patch (obsolete) — Splinter Review
Assignee: nobody → madperson

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.

Crash Signature: [@ oly64.dll | RtlProcessFlsData | LdrShutdownThread] [@ oly64.dll | RtlpFlsDataCleanup | LdrShutdownThread] [@ oly64.dll] [@ oly.dll] → [@ oly64.dll | RtlProcessFlsData | LdrShutdownThread] [@ oly64.dll | RtlpFlsDataCleanup | LdrShutdownThread] [@ oly64.dll] [@ oly.dll | LdrShutdownThread]

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

Blocks: 1435797
Flags: needinfo?(mcastelluccio)

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.

Crash Signature: [@ oly64.dll | RtlProcessFlsData | LdrShutdownThread] [@ oly64.dll | RtlpFlsDataCleanup | LdrShutdownThread] [@ oly64.dll] [@ oly.dll | LdrShutdownThread] → [@ oly64.dll | RtlProcessFlsData | LdrShutdownThread] [@ oly64.dll | RtlpFlsDataCleanup | LdrShutdownThread] [@ oly64.dll] [@ oly.dll | LdrShutdownThread] [@ pdzipmenu64.dll | RtlProcessFlsData | LdrShutdownThread] [@ pdzipmenu32.dll | LdrShutdownThr…
Attached patch bug1576728.patch (obsolete) — Splinter Review
Attachment #9088253 - Attachment is obsolete: true
Attachment #9093620 - Flags: review?(mcastelluccio)
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.
Attachment #9093620 - Flags: review?(mcastelluccio)
Attachment #9093620 - Flags: review?(aklotz)
Attachment #9093620 - Flags: review+
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.
Attachment #9093620 - Flags: review?(aklotz) → review-
Attached patch bug1576728.patch (obsolete) — Splinter Review

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?

Attachment #9093620 - Attachment is obsolete: true
Attachment #9094617 - Flags: review?(aklotz)

(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 on attachment 9094617 [details] [diff] [review]
bug1576728.patch

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

Thanks
Attachment #9094617 - Flags: review?(aklotz) → review+

Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ba05fadf202
Add crashing dlls from chinese unzipping tools to the blocklist. r=aklotz

Keywords: checkin-needed

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

Keywords: regression

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.

Priority: -- → P2

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.

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...

Flags: needinfo?(aklotz)

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

Flags: needinfo?(aklotz) → needinfo?(tkikuchi)

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.

Flags: needinfo?(tkikuchi)

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.

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

Flags: needinfo?(madperson)

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

Flags: needinfo?(madperson)

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.

thanks for the clarification.

Attachment #9094617 - Attachment is obsolete: true
Flags: needinfo?(madperson)
Attachment #9107505 - Flags: review?(aklotz)

Do you mind using phabricator for this?

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

Do you mind using phabricator for this?

Flags: needinfo?(madperson)

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...

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

Flags: needinfo?(madperson) → needinfo?(tkikuchi)

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

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

Done!

Flags: needinfo?(tkikuchi)
Comment on attachment 9107505 [details] [diff] [review]
bug1576728_version.patch

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

Moved to phabricator.
Attachment #9107505 - Flags: review?(aklotz)
Pushed by apavel@mozilla.com:
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

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

Flags: needinfo?(gpascutto)

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

Flags: needinfo?(gpascutto) → needinfo?(tkikuchi)

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
...
Flags: needinfo?(tkikuchi)

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

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b8199ac5ba9
Block more versions of oly[64].dll and pdzipmenu[32|64].dll.  r=gcp
Status: NEW → RESOLVED
Closed: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.