False positive: This expression has no effect (because _function with multiple implementations_ has no external side effects [in incorrectly selected version]).

[jsoref]

Description of the false positive

https://github.com/PowerDNS/pdns/pull/16363#discussion_r2455671092

This expression has no effect (because sendUDPResponse has no external side effects).

codeql appears to have found https://github.com/PowerDNS/pdns/blob/dec9583d885713a0d0ecb55a74ef83cde5f8a235/pdns/dnsdistdist/test-dnsdist_cc.cc#L83 (which is indeed side effect free) instead of https://github.com/PowerDNS/pdns/blob/dec9583d885713a0d0ecb55a74ef83cde5f8a235/pdns/dnsdistdist/dnsdist.cc#L637 (which shouldn't be)

Code samples or links to source code

https://github.com/pieterlexis/pdns/blob/064c4c9f2ea60c59d4b0c1a95daad4e17cbe2ce4/pdns/dnsdistdist/dnsdist-udp.cc#L131

https://github.com/PowerDNS/pdns/blob/dec9583d885713a0d0ecb55a74ef83cde5f8a235/pdns/dnsdistdist/test-dnsdist_cc.cc#L83 https://github.com/PowerDNS/pdns/blob/dec9583d885713a0d0ecb55a74ef83cde5f8a235/pdns/dnsdistdist/dnsdist.cc#L637

URL to the alert on GitHub code scanning (optional)

I don't have access to this, but I'll see if I can get someone to provide it.

[hvitved]

Thank you for reporting this. I'll inform the relevant team.

[Habbie]

https://github.com/pieterlexis/pdns/blob/064c4c9f2ea60c59d4b0c1a95daad4e17cbe2ce4/pdns/dnsdistdist/dnsdist-udp.cc#L131

https://github.com/PowerDNS/pdns/security/code-scanning/23920

can't easily find the other two

[jketema]

Hi @jsoref,

Thanks for the report. I had a closer look at this, and these is not false positives, although these alerts are definitely counter intuitive.

What is going on here is that the created CodeQL database contains information on all the binaries that were built. In your case this includes both the dnsdist binary and the testrunner binary. This data is nicely compartmentalized in the database to avoid confusing alerts, but alerts may still come from either of the binaries, as the query in question does not attempt to filter out alerts from test binaries. When I dig into the alerts, I can see that they are all coming from the testrunner binary, and it the context of that binary this is clearly a true positive.

Unfortunately, improving the query to suppress these seemingly false positives is currently not a product priority. From this perspective your options are either:

• dismiss the alert, ideally as "Used in tests", or
• do not build the test code as part of your CodeQL analysis workflow.

Of course the disadvantage of the latter approach is that there might be useful alerts coming from your test code, which would then be missed.

[jsoref]

Thanks for looking.

From my perspective, either:

• the parsing should be a global thing and decide that there is a code path where there is a side effect (the dnsdist binary); or
• the error should report which binary it's using (testrunner) - bonus points for only mentioning it when there's a path for a different binary (dnsdist) that doesn't have this behavior.

[jketema]

the parsing should be a global thing and decide that there is a code path where there is a side effect (the dnsdist binary); or

That requires us to make a decision on what binary is important to you, which in this case seems straightforward (dnsdist), but might not be straightforward in general, and an alert we filter out might consequently be of real interest. So, yes, we could think about heuristics, but there will always be cases where those will fail either leading to false positives or false negatives.

the error should report which binary it's using (testrunner) - bonus points for only mentioning it when there's a path for a different binary (dnsdist) that doesn't have this behavior.

This is definitely a possible approach to addressing this issue.