1794249 - AddressSanitizer: heap-use-after-free [@ mozilla::dom::workerinternals::loader::CacheLoadHandler::Fail] with WRITE of size 4

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 107.0a1-20221004213513-https://hg.mozilla.org/mozilla-central/rev/73c16d284362ba24606a516cd454dd3fe395b9b6.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Yulia Startsev [:yulia]]

It looks like this is related to 1792984

Comment 4

[Daniel Veditz [:dveditz]]

Might turn out to be a dupe, but rating it appropriately in case it's not.

Comment 5

[Jens Stutte [:jstutte]]

Yulia, assigning this to you for further monitoring (and to make clear we have it on our radar).

Comment 6

[Donal Meehan [:dmeehan]]

:yulia, was it confirmed that this is a duplicate of Bug 1792984?

Comment 7

[Yulia Startsev [:yulia]]

I believe this might still be possible to trigger in spite of 1792984, though I don't know if it is still happening. In particular if the promise resolves very late, then loadContext will be dereferenced. We are failing on this line: https://searchfox.org/mozilla-central/rev/b0e929cfaf3994cad2dd02afdac138083ee3fc84/dom/workers/loader/CacheLoadHandler.cpp#263

This was something I was wary about before, and there is an unrelated patch that fixes this on https://bugzilla.mozilla.org/show_bug.cgi?id=1797327 -- I will ping asuth there.

Comment 8

[Jens Stutte [:jstutte]]

IIUC, bug 1797327 has been fixed now. Can we consider this solved then?

:yulia, :asuth, please give an update here.

Comment 9

[Yulia Startsev [:yulia]]

This should be resolved, it is impossible to get into this state now, as we are holding a strong ref to the worker context.

Comment 10

[Yulia Startsev [:yulia]]

This shouldn't have been closed, sorry -- will let the security team do that.

Comment 11

[Jens Stutte [:jstutte]]

Not sure what the security team can do here now other than closing - the patch on bug 1797327 already landed (without sec-approval, and probably it did not need it). IIUC we cannot DUPE it to keep it secret, can we?

Comment 12

[Daniel Veditz [:dveditz]]

You can, now, set bug relations like duplicate and depends on and still keep a bug relatively secret. For a while these relations were totally invisible to anyone who couldn't see the security bug, but that was causing coordination problems so we've backed off and let the relationships be visible to folks with editbugs permission (as before, showing only the bug-id and status).

Duplicate wouldn't be the right resolution, though, because that other bug was not a security bug. In those cases "depends on" is the right relationship. Many times the public bug is some larger piece of work that we believe will fix the security bug: that's a true "depends on" case. Other times when it is more clearly the exact same issue, marking it a duplicate obscures important tracking information used to trigger security bug verification, generate advisories, and handle bug bounties among other things.

If we need to uplift this fix to beta or ESR branches we would use this security bug to track those releases.

Yulia: is this bug also a regression from Bug 1742438 like the see-also bug 1792984? If so, we don't need the fix on ESR. Given it's RC week and bug 1797327 has been blamed for a couple of regressions we should let this ride the trains and not try to push it into 107-Beta.

Comment 13

[Daniel Veditz [:dveditz]]

Decoder: did we ever see this signature much in ASAN nightly? If so, has it disappeared from builds since Nov 2 or so? Given the discovery date of this bug is much after the possible regressor maybe there's no way to verify this and we should just presume it's fixed.

Comment 14

[Yulia Startsev [:yulia]]

Yes that is right. This is a regression from https://bugzilla.mozilla.org/show_bug.cgi?id=1742438.

Comment 15

[Jens Stutte [:jstutte]]

Yulia, so IIUC from comment 12 we can just make this depend on bug 1797327 and close it FIXED?

Comment 16

[Yulia Startsev [:yulia]]

Yes, this one can be closed as duplicate as this state isn't possible any more.

Comment 17

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

Set release status flags based on info from the regressing bug 1742438