Skip to content

Support distinct handling and configuration for DCHECK failures #5048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ArthurSonzogni wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support distinct handling and configuration for DCHECK failures #5048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/clusterfuzz/_internal/crash_analysis/crash_analyzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,16 +320,21 @@ def is_security_issue(crash_stacktrace, crash_type, crash_address):
return True

if crash_type == 'CHECK failure':
# TODO(ochang): Remove this once we pick up newer builds that distinguish
# DCHECKs from CHECKs.
checks_have_security_implication = environment.get_value(
'CHECKS_HAVE_SECURITY_IMPLICATION', False)
return checks_have_security_implication

# Debug CHECK failure should be marked with security implications.
if crash_type in ('Security DCHECK failure', 'DCHECK failure'):
# Security DCHECKs are always security issues.
if crash_type == 'Security DCHECK failure':
return True

# For regular DCHECKs, projects can choose whether or not a particular fuzzer
# job treats them as security issues.
Comment on lines +331 to +332
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terms "fuzzer" and "job" have specific meanings in ClusterFuzz (and FuzzerJob is also a thing), so let's reword:

Suggested change
# 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.

if crash_type == 'DCHECK failure':
dchecks_have_security_implication = environment.get_value(
'DCHECKS_HAVE_SECURITY_IMPLICATION', True)
return dchecks_have_security_implication

# Hard crash, explicitly enforced in code.
if (crash_type == 'Fatal error' or crash_type == 'Unreachable code' or
crash_type.endswith('Exception') or crash_type.endswith('CHECK failure')):
Expand Down
4 changes: 3 additions & 1 deletion src/clusterfuzz/_internal/issue_management/issue_filer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,9 @@ def file_issue(testcase,
is_crash = not utils.sub_string_exists_in(NON_CRASH_TYPES,
testcase.crash_type)
properties = policy.get_new_issue_properties(
is_security=testcase.security_flag, is_crash=is_crash)
is_security=testcase.security_flag,
is_crash=is_crash,
crash_type=testcase.crash_type)

issue = issue_tracker.new_issue()
issue.title = data_handler.get_issue_summary(testcase)
Expand Down
80 changes: 49 additions & 31 deletions src/clusterfuzz/_internal/issue_management/issue_tracker_policy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,62 +146,80 @@ def fallback_policy_message(self):
"""Get the fallback policy message, if it exists."""
return self._data.get('fallback_policy_message')

def get_new_issue_properties(self, is_security, is_crash):
def get_new_issue_properties(self, is_security, is_crash, crash_type):
"""Get the properties to apply to a new issue."""
policy = NewIssuePolicy()

if 'all' in self._data:
self._apply_new_issue_properties(policy, self._data['all'], is_crash)
properties = {
'is_security': is_security,
'is_crash': is_crash,
'is_dcheck': 'DCHECK failure' in crash_type
}

self._apply_new_issue_properties(policy, self._data.get('all'),
**properties)

# Legacy support: support top-level 'security' and 'non_security' keys. New
# configs should use the 'all' section and add 'security' and
# 'non_security' subsections within that.
if is_security:
if 'security' in self._data:
self._apply_new_issue_properties(policy, self._data['security'],
is_crash)
self._apply_new_issue_properties(policy, self._data.get('security'),
**properties)
else:
if 'non_security' in self._data:
self._apply_new_issue_properties(policy, self._data['non_security'],
is_crash)
self._apply_new_issue_properties(policy, self._data.get('non_security'),
**properties)

return policy

def _apply_new_issue_properties(self, policy, issue_type, is_crash):
def _apply_new_issue_properties(self, policy, data, **properties):
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"""Apply issue policies."""
Comment on lines +174 to 175
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here and understand how this code works probably better than anyone else in the world, let's document it:

Suggested change
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 not issue_type:
if not data:
return

if 'status' in issue_type:
policy.status = self._data['status'][issue_type['status']]
if 'status' in data:
policy.status = self._data['status'][data['status']]

if 'ccs' in issue_type:
policy.ccs.extend(issue_type['ccs'])
if 'ccs' in data:
policy.ccs.extend(data['ccs'])
Comment on lines +182 to +183
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

Suggested change
if 'ccs' in data:
policy.ccs.extend(data['ccs'])
policy.ccs.extend(data.get('ccs', []))


labels = issue_type.get('labels')
labels = data.get('labels')
if labels:
policy.labels.extend(_to_str_list(labels))

issue_body_footer = issue_type.get('issue_body_footer')
issue_body_footer = data.get('issue_body_footer')
if issue_body_footer:
policy.issue_body_footer = issue_body_footer

if is_crash:
crash_labels = issue_type.get('crash_labels')
if crash_labels:
policy.labels.extend(_to_str_list(crash_labels))
else:
non_crash_labels = issue_type.get('non_crash_labels')
if non_crash_labels:
policy.labels.extend(_to_str_list(non_crash_labels))

for k, v in self.get_extension_fields(issue_type).items():
for k, v in self.get_extension_fields(data).items():
policy.extension_fields[k] = v

# Handle conditions recursively:
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)
Comment on lines +197 to +207
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Legacy support: support "crash_label" and "non_crash_label" keys.
# New configs should use the "crash" and "non_crash" sections instead and
# a "labels" list within those sections.
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:
Comment on lines +212 to +214
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do is False and is True explicitly instead of relying on None being falsey?

policy.labels.append(str(data['non_crash_label']))

def get_existing_issue_properties(self):
"""Get the properties to apply to a new issue."""
policy = NewIssuePolicy()

if 'existing' in self._data:
self._apply_new_issue_properties(policy, self._data['existing'], False)

properties = {} # No recursive properties for existing issues.
Copy link
Copy Markdown
Collaborator

@letitz letitz Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the right comment here? And are we replicating previous behavior? We used to pass is_crash = False explicitly.

self._apply_new_issue_properties(policy, self._data['existing'],
**properties)
return policy


Expand Down
Loading
Loading