Rollup rlbox, wasm2c improvements for hunspell sandbox to beta
Categories
(Core :: Security: RLBox, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox95 | --- | fixed |
People
(Reporter: shravanrn, Assigned: shravanrn)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
|
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
This roll up contains minimized versions of the following bugs/patches
Bug 1740399 - Ensure hunspell fails gracefully on rlbox sandbox OOM
Bug 1740353 - Configure hunspell rlbox sandbox so AMO dictionaries have a smaller footprint
Bug 1740187 - Annotate crash reports with rlbox sandbox malloc failures
Bug 1739669 - Configure hunspell rlbox sandbox for higher mem size depending on locale
Bug 1739762 - Update rlbox_wasm2c to support variable sized sandboxes in 32-bit platforms
|
Assignee | |
Comment 1•3 years ago
|
||
|
||
Comment 2•3 years ago
|
||
Comment on attachment 9250551 [details]
Bug 1740974 - Rollup rlbox, wasm2c improvements for hunspell sandbox to beta r=bholley
Beta/Release Uplift Approval Request
- User impact if declined: We're aiming to make a coordinated announcement in Firefox 95 of WASM-based sandboxing of multiple libraries across all supported platforms. One of those libraries was intended to be hunspell, but we disabled hunspell sandboxing on 32-bit windows in bug 1739669 due to crashes on rare locales (e.g. Bulgarian) with very large spelling dictionaries. Our Nightly audience appears to small to surface these issues, but we have reproduced the issue locally and written patches to fix the issue. These patches dynamically size the sandbox based on dictionary size, allowing locales like Bulgarian to reserve more memory for spellchecking than our common locales.
When the above crashes surfaced on socorro, we also saw another similar crash signature, bug 1736171, which we could not reproduce locally but suspect is another manifestation of the same underlying problem. The changes here also include some additional diagnostics so that, should the crashes in bug 1736171 reappear, we will learn more about them.
Because having the hunspell sandbox not enabled on 32-bit windows would require an unfortunate asterisk in our messaging, and because we still have several weeks remaining on the beta cycle, I think we should take this patch.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The main risk here is that we'd somehow break spellchecking, either in terms of functionality or in terms of crashes. This has been on Nightly for the better part of a week and we haven't seen either, so if there are problems, they're likely to be in the kinds of locales that we need beta to reach.
- String changes made/needed:
|
|
||
Comment 3•3 years ago
|
||
Note that the uplift here should also include relanding the config change in bug 1737704, and setting the tracking flags in that bug appropriately. I asked Shravan not to include that in the rollup so that bug 1737704 can serve as the source of truth for whether 32-bit hunspell rlbox is enabled or not.
|
|
Assignee | |
Comment 4•3 years ago
•
|
||
I tested win32 nightly
on text fields with spellcheck and language specified
en firefox with addon dictionaries fr, ru, bg, sr, pl, oc
on text fields with spellcheck and default language
en firefox
bg firefox
sr firefox
oc firefox
All spellcheck work as expected with no crashes
|
||
Comment 5•3 years ago
|
||
Comment on attachment 9250551 [details]
Bug 1740974 - Rollup rlbox, wasm2c improvements for hunspell sandbox to beta r=bholley
Approved for 95 beta 8, thanks.
|
|
||
Comment 6•3 years ago
|
||
| bugherder uplift | ||
|
|
||
Updated•3 years ago
|
|
|
||
Comment 7•3 years ago
|
||
We're aiming to make a coordinated announcement in Firefox 95 of WASM-based sandboxing of multiple libraries across all supported platforms.
Should that be part of our Firefox 95 release notes (beta release notes once it is in beta8 and final release notes) or will it be mentioned on our 95 developer notes on MDN?, Thanks
|
|
||
Comment 8•3 years ago
|
||
(In reply to Pascal Chevrel:pascalc from comment #7)
We're aiming to make a coordinated announcement in Firefox 95 of WASM-based sandboxing of multiple libraries across all supported platforms.
Should that be part of our Firefox 95 release notes (beta release notes once it is in beta8 and final release notes) or will it be mentioned on our 95 developer notes on MDN?, Thanks
This should have no impact on the behavior of the platform, it’s purely a security measure for users. So I assume that’s release notes.
Description
•