1717029 - Missing catch-all in enum comparison in FontFaceSet.cpp

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

As a result of some static analysis work, we found a situation where we compare against at least two values of an enum, but lack a catch-all case.

This happens in https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/layout/style/FontFaceSet.cpp#1675-1681:

    if (f->Status() == FontFaceLoadStatus::Loaded) {
      loaded.AppendElement(*f);
      mNonRuleFaces[i].mLoadEventShouldFire = false;
    } else if (f->Status() == FontFaceLoadStatus::Error) {
      failed.AppendElement(*f);
      mNonRuleFaces[i].mLoadEventShouldFire = false;
    }

As a result of a survey sent to assignees of sec-high bugs (you might have gotten it yourself) we are exploring static analysis queries that may help prevent future bugs. The blocking bug details one of these queries. This bug is filed (manually) as a result of running that query on m-c and randomly selecting 10 hits to investigate. (Because it would bias the experiment, no subjective analysis was done on the particular code in question.)

We'd appreciate your help in triaging this report, and indicating which of the following cases apply:

• If the code is intentional - that is there should not be a catch-all at the end and fallthrough for all current enum values is safe and bug-free, we'd ask you close this as WONTFIX
• If the code is currently safe (e.g. it checks all the enum values), but could be improved to prevent future bugs (caused by an addition to the enum) with a catch-all and a DIAGNOSTIC/RELEASE assert.
• If the code is believed safe but can't be locally asserted to be so (e.g. it handles 2 of 5 enum values and the remaining three "should never occur") - this is the most interesting case to us. If the remaining cases "should never occur" then it should be safe to add an assert, and this could lead us to prevent or catch a future functional or security bug (just like Bug 1603055).

Comment 1

[Jonathan Kew [:jfkthame]]

The code here is intentional and safe as it stands; the values it doesn't check are ones where nothing needs to be done.

Still, since it's been pointed out, I think we could make it a little more explicit and robust against potential changes by using a switch statement that explicitly handles all the current values, including those that are no-ops, and adding a default with MOZ_ASSERT_UNREACHABLE to catch any future extension of the enum.

So no change in functionality needed, but we can usefully clarify the code.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1717029 - Clean up status checks in FontFaceSet::CheckLoadingFinished(). r=#layout-reviewers

No functional change, this just makes it more explicit that each possible status
value is being considered, and unifies the handling of the two arrays.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88c320350f41
Clean up status checks in FontFaceSet::CheckLoadingFinished(). r=layout-reviewers,emilio

Comment 4

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/88c320350f41