Support distinct handling and configuration for DCHECK failures #5048
Conversation
ArthurSonzogni
force-pushed
the
dcheck-config
branch
2 times, most recently
from
f17652a to
f1d73d3
Compare
Separates `DCHECK` failures from standard `CHECK` failures to enable granular severity assessment and issue tracking policies. In Chromium, `DCHECK` failures often carry different security and priority implications than production `CHECK` failures. While they may not always be treated as immediate security vulnerabilities, they present information disclosure risks if filed publicly. Current logic groups them together, preventing distinct visibility rules. Detailed changes: - **Stack Parsing:** Updates `stacktraces` regex constants to explicitly distinguish "DCHECK failed" from "Check failed/NOTREACHED", assigning the distinct crash type `DCHECK failure`. - **Security Implications:** Introduces the `DCHECKS_HAVE_SECURITY_IMPLICATION` environment variable to control whether DCHECKs are flagged as security issues per-fuzzer. - **Policy Engine:** Refactors `IssueTrackerPolicy` to support recursive configuration application. This allows nested conditions (e.g., `all` -> `non_security` -> `dcheck`) to apply specific labels, access limits, or priority levels based on the intersection of crash traits. This decouple the configuration depth from the code, enabling arbitrary nesting or rules and simplifying the addition of future condition types. Bug: https://issues.chromium.org/issues/406667202
ArthurSonzogni
force-pushed
the
dcheck-config
branch
from
f1d73d3 to
18020c3
Compare
|
Hey @letitz The review is a low priority. I can wait as long as needed. |
letitz
left a comment
letitz
left a comment
There was a problem hiding this comment.
Looking good overall, a couple comments about specifics.
| # For regular DCHECKs, projects can choose whether or not a particular fuzzer | ||
| # job treats them as security issues. |
There was a problem hiding this comment.
The terms "fuzzer" and "job" have specific meanings in ClusterFuzz (and FuzzerJob is also a thing), so let's reword:
| # For regular DCHECKs, projects can choose whether or not a particular fuzzer | |
| # job treats them as security issues. | |
| # Regular DCHECKs are considered security issues by default, but users | |
| # can invert that if they so desire. |
| return policy | ||
|
|
||
| def _apply_new_issue_properties(self, policy, issue_type, is_crash): | ||
| def _apply_new_issue_properties(self, policy, data, **properties): |
There was a problem hiding this comment.
I would rather keep explicit arguments (named or not, perhaps even wrapped in a dataclass if the list of arguments is getting unwieldy) than this, we lose type information here. AFAICT, properties always has the same 3 keys?
| def _apply_new_issue_properties(self, policy, data, **properties): | ||
| """Apply issue policies.""" |
There was a problem hiding this comment.
While we're here and understand how this code works probably better than anyone else in the world, let's document it:
| def _apply_new_issue_properties(self, policy, data, **properties): | |
| """Apply issue policies.""" | |
| def _apply_new_issue_properties(self, policy: NewIssuePolicy, data, **properties): | |
| """Fill in `policy` with properties that an issue linked to a given testcase should possess. | |
| policy: Output argument. | |
| data: configuration that applies to the testcase linked to the issue. | |
| properties: Properties of the testcase linked to the issue. | |
| """ |
| if 'ccs' in data: | ||
| policy.ccs.extend(data['ccs']) |
There was a problem hiding this comment.
Optional:
| if 'ccs' in data: | |
| policy.ccs.extend(data['ccs']) | |
| policy.ccs.extend(data.get('ccs', [])) |
| children_conditions = { | ||
| 'security': properties.get('is_security', None) is True, | ||
| 'non_security': properties.get('is_security', None) is False, | ||
| 'crash': properties.get('is_crash', None) is True, | ||
| 'non_crash': properties.get('is_crash', None) is False, | ||
| 'dcheck': properties.get('is_dcheck', None) is True, | ||
| 'non_dcheck': properties.get('is_dcheck', None) is False, | ||
| } | ||
| for child, condition in children_conditions.items(): | ||
| if condition: | ||
| self._apply_new_issue_properties(policy, data.get(child), **properties) |
There was a problem hiding this comment.
I think spelling this out imperatively would be easier to read:
if properties.get('is_security'):
self._apply_new_issue_properties(policy, data.get('security'), **properties)
# etc.| if properties.get('is_crash', None) is False and 'crash_label' in data: | ||
| policy.labels.append(str(data['crash_label'])) | ||
| if properties.get('is_crash', None) is True and 'non_crash_label' in data: |
There was a problem hiding this comment.
Why do we do is False and is True explicitly instead of relying on None being falsey?
| if 'existing' in self._data: | ||
| self._apply_new_issue_properties(policy, self._data['existing'], False) | ||
|
|
||
| properties = {} # No recursive properties for existing issues. |
There was a problem hiding this comment.
Is this really the right comment here? And are we replicating previous behavior? We used to pass is_crash = False explicitly.
2 participants
Separates
DCHECKfailures from standardCHECKfailures to enable granular severity assessment and issue tracking policies.In Chromium,
DCHECKfailures often carry different security and priority implications than productionCHECKfailures. While they may not always be treated as immediate security vulnerabilities, they present information disclosure risks if filed publicly. Current logic groups them together, preventing distinct visibility rules.Detailed changes:
stacktracesregex constants to explicitly distinguish "DCHECK failed" from "Check failed/NOTREACHED", assigning the distinct crash typeDCHECK failure.DCHECKS_HAVE_SECURITY_IMPLICATIONenvironment variable to control whether DCHECKs are flagged as security issues per-fuzzer.IssueTrackerPolicyto support recursive configuration application. This allows nested conditions (e.g.,all->non_security->dcheck) to apply specific labels, access limits, or priority levels based on the intersection of crash traits. This decouple the configuration depth from the code, enabling arbitrary nesting or rules and simplifying the addition of future condition types.Bug: https://issues.chromium.org/issues/406667202