Undefined behavior in zxx_stream::zxx_stream()

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: mozillabugs, Assigned: glandium)

Tracking

59 Branch
mozilla62
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(1 attachment)

zxx_stream::zxx_stream() (mozglue\linker\zip.h) initializes itself in part using memset():

   40: memset (this, 0, sizeof(z_stream));

where |z_stream| is a base class of |zxx_stream|. This code invokes undefined behavior, since it assumes that the base class subobject resides at |this|, which contravenes C++11 s.10:

------
[t]he order in which the base class subobjects are allocated in the most derived object (1.8) is unspecified."
------

zxx_stream() appears to be used in Android builds only.
Flags: sec-bounty?
Group: core-security → dom-core-security
Mike, can you please take a look at this and weigh in on the severity?
Practically speaking, it doesn't matter. The worse that could happen is the allocator field being overwritten with zeroes, which would only mean zlib would be using new/delete instead of the given allocator. But in practice that doesn't happen anyways.
Flags: needinfo?(mh+mozilla)
And in fact, the only thing that was using the StaticAllocator was removed in bug 1376704, so a lot of this can actually be removed.
I have a patch to remove the whole class. Can this bug be unflagged as confidential so that I can push it to mozreview?
Group: dom-core-security
Assignee: nobody → mh+mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: sec-bounty?
https://hg.mozilla.org/mozilla-central/rev/ba2417447483
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.