CodeQL - false positive - JPL Rule 24

[ArielSAdamsNASA]

CodeQL is throwing an error for multiple variable declarations on one line when there is not multiple declarations.

    }

    *PortNum = ntohs(sa_port);

    return OS_SUCCESS;
} /* end OS_SocketAddrGetPort_Impl */

Link: https://github.com/ArielSAdamsNASA/osal/security/code-scanning/22128?query=ref%3Arefs%2Fheads%2FJPL-24

    OS_CHECK_POINTER(timer_id);
    OS_CHECK_APINAME(timer_name);
    OS_CHECK_POINTER(accuracy);
    OS_CHECK_POINTER(callback_ptr);

Link: https://github.com/ArielSAdamsNASA/osal/security/code-scanning/19301?query=ref%3Arefs%2Fheads%2FJPL-24

[tausbn]

Thank you for your report! I'll hand this off to the C/C++ team for further comment.

[jbj]

We can't follow the links as we don't have access. My guess is that technically there are multiple declarations on one line in the examples you've written, but they're hidden behind macros. It's common to implement ntohs as a macro, and the all-upper-case OS_CHECK_POINTER also looks like a macro.

The query giving these errors is https://github.com/github/codeql/blob/main/cpp/ql/src/JPL_C/LOC-4/Rule%2024/MultipleVarDeclsPerLine.ql.

Given that you have strict rules for code clarity (which is why you're using that query), are you certain that you're comfortable using macros that declare variables? They can cause macro hygiene problems. The ntohs macro can easily be rewritten as an inline function, and a modern compiler should optimize it to give the same code as the macro. I don't know about the OS_CHECK_POINTER macro.

In both cases, you can work around the error by replacing

int a = x,
    b = y;

in the macro definitions with

int a = x;
int b = y;

Does this advice help? Otherwise we can consider changing the query to ignore code that comes from macros, but that might hurt you if you want to err on the side of caution with these queries.

[ArielSAdamsNASA]

For the rule, multiple variable declarations, would it be possible to ignore code from macros that comes from system headers, external macros, rather than all macros?

Then for the rule, more than one statement per line, could you ignore macros completely? These errors are thrown for function-like macros as seen below with OS_CHECK_APINAME(timer_name);.

#define OS_CHECK_APINAME(str) OS_CHECK_STRING(str, OS_MAX_API_NAME, OS_ERR_NAME_TOO_LONG)

[jbj]

Thank you for your patience, @ArielSAdamsNASA. We've been discussing internally how much we can weaken our JPL rules. They're strict and Draconian, and our original consumers of those rules wanted them that way. Because of how macros work in C, it can also be hard to tell the difference between what generated code comes from the macro itself and what comes from its arguments.

Excluding system headers sounds like an adjustment we can safely make since generally LGTM does not display alerts in system headers. Excluding all macros from "more than one statement per line" might violate JPL Rule 20 (preprocessor use) "Use of the C preprocessor shall be limited to file inclusion and simple macros."

[jbj]

I've opened #6723 as a possible solution, but we're trying to get in touch with all users of these queries before merging a change that may not be universally accepted.

[jbj]

We could not get support for this change from all users of these queries, so I've closed https://github.com/github/codeql/pull/6723 in favour of https://github.com/github/codeql/pull/7014. The latter PR just changes the libraries and leaves it up to users to change the queries to use them.

[skliper]

It looks to me like we are getting this warning on inline code for something like:

x = inlinefcn(y);

with the inline being very simple... something like:

static inline int inlinefcn(int input)
{
  return input+1;
}

Note I didn't try this exact code example above, but the code we are seeing the error on is really pretty simple: https://github.com/nasa/osal/blob/6e6afb4d3ea611f5d11e4e94a5adee595cc08b99/src/os/shared/src/osapi-time.c#L447 https://github.com/nasa/osal/blob/6e6afb4d3ea611f5d11e4e94a5adee595cc08b99/src/os/shared/inc/os-shared-idmap.h#L347-L350

Is this really expected/intended to throw the warning in this case?

[jketema]

@skliper I'm not able to reproduce the behaviour you're describing. I built the library per the instructions near the bottom of the readme:

mkdir build_osal
cd build_osal
cmake -DOSAL_SYSTEM_BSPTYPE=generic-linux ..
make

creating a database while running make. Running cpp/jpl-c/multiple-var-decls-per-line on the database gives me 0 results.

[skliper]

@jketema Thanks for looking into it. I'm also unable to recreate the case where I saw warnings on the inline functions.