Open Bug 1576728 Opened 1 year ago Updated 3 months ago

Crash in oly64.dll (FYunZip)

Categories

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

All
Windows
defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 affected, firefox67 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 fix-optional)

Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- affected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fix-optional

People

(Reporter: philipp, Assigned: philipp)

References

(Blocks 1 open bug)

Details

(Keywords: crash, leave-open)

Crash Data

Attachments

(2 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
You need to log in before you can comment on or make changes to this bug.