github / codeql-coding-standards

This repository contains CodeQL queries and libraries which support various Coding Standards.
MIT License
121 stars 48 forks source link

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

Closed rak3-sh closed 1 month ago

rak3-sh commented 1 month ago

Affected rules

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 commented 1 month ago

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 commented 1 month ago

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