Skip to content
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.

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

PRE32-C: Raises false positives when a file has a function call and a preprocessor directive at any location. #650

Closed
rak3-sh opened this issue Jul 26, 2024 · 2 comments
Labels
Difficulty-Low A false positive or false negative report which is expected to take <1 day effort to address false positive/false negative An issue related to observed false positives or false negatives. Impact-Low user-report Issue reported by an end user of CodeQL Coding Standards

Comments

@rak3-sh
Copy link
Contributor

rak3-sh commented Jul 26, 2024

Affected rules

  • PRE32-C

Description

This rule should check the use of preprocessor directives in arguments in a function call but it actually raises an alert even when the preprocessor directive is outside the function call.

Example

void func1(void) {
 // function body
}

int foo(const int arg) noexcept {
   int x{arg};
   func1(); // function call
#if MACRO == 7 // Preprocessor directive after the function call.
#endif
    return x;
}

int main()
{
  return 0;
}
@rak3-sh rak3-sh added the false positive/false negative An issue related to observed false positives or false negatives. label Jul 26, 2024
@rak3-sh
Copy link
Contributor Author

rak3-sh commented Jul 26, 2024

Root Cause

The root cause is a weak heuristic to identify the location of the preprocessor directive and the function call's end line. It uses the isFunctionSuccessorLocation predicate to check for the location of the successor of the function call which in this case becomes the return statement(?) and hence, when it checks whether the macro is before the function call's end line, it holds true and the false positive is raised.

Fix Strategy

Since the rule mentions usage of preprocessor directives in function call arguments, we can leverage the argument's location instead of the function call's successor's line information. We can look for a function call's argument which is after the preprocessor directive's line number. Hence, changing the isLocatedInAFunctionInvocation predicate like below, we can fix the false positives and find the intended calls which have preprocessor directives inside.

/**
 * Find a preprocessor directive nestled in between the
 * function call's start line and its argument's start line.
 * E.g.
 *  memcpy(dest, src, // function call
 *  #ifdef PLATFORM1 //preprocessor directive
 *  12 // argument
 *  #else
 *  24
 *  #endif
 *  );
 */
PreprocessorDirective isLocatedInAFunctionInvocation(FunctionCall c) {
  exists(PreprocessorDirective p, File f, int startCall, int endCall |
    isFunctionInvocationLocation(c, f, startCall, endCall) and
    exists(Expr arg, int preprocStartLine, int preprocEndLine |
      c.getAnArgument() = arg and
      isPreprocDirectiveLine(p, f, preprocStartLine, preprocEndLine) and
      // function call begins before preprocessor directive
      startCall < preprocStartLine and
      startCall < preprocEndLine and
      // argument is after preprocessor directive
      arg.getLocation().getStartLine() > preprocStartLine and
      // function call ends after preprocessor directive
      endCall > preprocEndLine
    ) and
    result = p
  )
}

@lcartey lcartey added user-report Issue reported by an end user of CodeQL Coding Standards Difficulty-Low A false positive or false negative report which is expected to take <1 day effort to address Impact-Low labels Jul 26, 2024
@lcartey lcartey moved this from Reported to Triaged in Coding Standards Public Development Board Jul 26, 2024
@lcartey
Copy link
Collaborator

lcartey commented Jul 26, 2024

Thanks @rak3-sh! That looks like a reasonable fix to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty-Low A false positive or false negative report which is expected to take <1 day effort to address false positive/false negative An issue related to observed false positives or false negatives. Impact-Low user-report Issue reported by an end user of CodeQL Coding Standards
Projects
Development

No branches or pull requests

2 participants