1886598 - (CVE-2024-4774) Undefined behavior in ShmemCharMapHashEntry

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[mozillabugs]

ShmemCharMapHashEntry (gfx/thebes/gfxPlatformFontList.h) invokes undefined behavior because it hasa mozilla::fontlist::Pointer which hasa std::atomic<uint32_t>, which is not guaranteed to be trivially copyable. Yet ShmemCharMapHashEntry is declared as ALLOW_MEMMOVE = true, which allows nsHashtable to move it using memmove() instead of by calling its move constructor/move assignment operator. memcpy/memmoveing is valid only for "trivially copyable" types; see C++ Standard N4750 s.6.7(1-3). Nothing in the Standard requires std::atomic to be trivially copyable, so this operation is UB. Indeed, std:: classes can be implemented in any manner that produces the correct observable behavior, N4750 s.4.1.1, unless explicitly documented otherwise.

Source from FIREFOX_123_0_1_RELEASE:

(gfx/thebes/gfxPlatformFontList.h):

 89: class ShmemCharMapHashEntry final : public PLDHashEntryHdr {
 ...
127:   enum { ALLOW_MEMMOVE = true };
128: 
129:  private:
130:   // charMaps are stored in the shared memory that FontList objects point to,
131:   // and are never deleted until the FontList (all referencing font lists,
132:   // actually) have gone away.
...
134:   mozilla::fontlist::Pointer mCharMap;
...
136: };

(gfx/thebes/SharedFontList.h):

 28: struct Pointer {
 ...
101:   // We store the block index and the offset within the block as a single
102:   // atomic 32-bit value so we can safely modify a Pointer without other
103:   // processes seeing a broken (partially-updated) value.
104:   std::atomic<uint32_t> mBlockAndOffset;
105: };

Comment 1

[Daniel Veditz [:dveditz]]

"moderate" may be overstating this because the actual compilers we use appear to implement atomics as trivially copyable in practice.

Comment 2

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

The severity field is not set for this bug.
:lsalzman, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

"moderate" may be overstating this because the actual compilers we use appear to implement atomics as trivially copyable in practice.

Yeah, it seems unlikely this is currently a "real" problem, but we should fix it anyway. Who knows what weird thing some future compiler might do.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1886598 - Struct with Pointer member may not be memmove-able. r=#gfx-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/323e403edaf0
Struct with Pointer member may not be memmove-able. r=gfx-reviewers,lsalzman

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/323e403edaf0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Doesn't feel like something we need to worry about backporting, but feel free to nominate for ESR approval if you feel strongly otherwise.

Comment 8

[Daniel Veditz [:dveditz]]

changing bug to sec-low, but we can bump it back up if it's shown that this is a realistic problem

Comment 9

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt