1717032 - Missing catch-all in enum comparison in StreamFilterParent.cpp

Status: RESOLVED WONTFIX

(WebExtensions :: Request Handling, defect)

Description

[Tom Ritter [:tjr]]

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/toolkit/components/extensions/webrequest/StreamFilterParent.cpp#302-307:

  if (mState == State::Suspended) {
    RefPtr<StreamFilterParent> self(this);
    RunOnMainThread(FUNC, [=] { self->mChannel->Resume(); });
  } else if (mState != State::TransferringData) {
    return IPC_OK();
  }

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

[Rob Wu [:robwu]]

The referenced code handles State::Suspended and State::TransferringData, with common logic in the end. It intentionally ignores other (non-relevant) states.