`STR34-C`: Rule improvements

Open
#577 0 comments 0 reactions 1 assignee View on GitHub

@lcartey is already working on this.

Since May 1, 2024.

Assessment

This issue has not been assessed yet.

Description

Difficulty-Medium false positive/false negative Impact-High Standard-CERT-C
Affected rules
  • STR34-C
Description
  • Do not consider specifiers when considering whether a type is a char type - whether a type is const, volatile etc. doesn't impact whether it's vulnerable to this bug.
  • Exclude cases where the range of the casted value does not contain negative values - this is because only negative signed char values are modified by the conversion to a larger signed integer.
  • Do not consider conversions to larger unsigned integers (they are excluded by the rule).
  • Do not report issues on platforms on which char is unsigned by default. Currently we say we want CharTypes but not UnsignedCharTypes, however that does not exclude the case where char is unsigned. I think we want the equivalent of c.getExpr().getType().(CharType).isSigned() (notwithstanding the first point in the list about specifiers)
  • Ignore implicit integer promotion conversions which occur as part of an equality or inequality comparison, where the other side of the comparison is also a signed char. In this specific case, the equality only holds if it would have held before the conversions.
  • We could also consider excluding the common pattern of (a >= 'A' && a <= ' F') and similar. These are safe as long as the two constants are within the range [0..CHAR_MAX].
  • We should also consider how to handle calls to library macros (such as tolower) which often create multiple results, which can be confusing to the user.
Example
void example_function(const char x) {
  if (x == EOF) ; // NON_COMPLIANT[FALSE_NEGATIVE] - missed because `x` is a `const char`

  if ('1' == EOF) ; // COMPLIANT[FALSE_POSITIVE] - assuming ASCII `1` can be represented by larger signed integral types, this is not a problem

  if (x == 1u) ; // Excluded from the rule by definition

  if (x == '~') ; // COMPLIANT - comparison valid - both sides have the same conversion applied
  if (x > '~') ; // NON_COMPLIANT - comparison isn't valid - `x` may be negative.
}
Dominant language
CodeQL
Stars
227
Forks
82
Avg merge
6d 7h
Merged PRs (30d)
9

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from github/codeql-coding-standards

All issues in github/codeql-coding-standards

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.