C++: Remove pointer/pointee conflation from parameter nodes

[MathiasVP]

Since node.hasAddressOperandAndIndirectionIndex(op, indirectionIndex) can hold for multiple (op, indirectionIndex) pairs for a given node (because a single DataFlow::Node can represent multiple Operands with different indirections) it's pretty much always a bug to only consider op or indirectionIndex in isolation. I thought we had fixed all problems like this as of #13425, but apparently this one was still lurking.

Luckily, the fix is simple 🎉

There are lots of DCA changes:

systemd

cpp/unbounded-write

The 7 lost results all go through the following:

int strv_push_with_size(char ***l, size_t *n, char *value) { // <-- (0)
 //
}

static inline int strv_push(char ***l, char *value) { // <-- (2)
        return strv_push_with_size(l, NULL, value); // <-- (1)
}

static int load_env_file_push_pairs(
                const char *filename, unsigned line,
                const char *key, char *value,
                void *userdata) { // <-- (4)

        char ***m = ASSERT_PTR(userdata); // equivalent to: m = userdata
        // ...

        if (value)
                return strv_push(m, value); // <--- (3)

That is: We track flow to ***l, but somehow end up tracking flow to **user_data.
So we magically lose an indirection here because of the conflation.

cpp/use-after-free

Both lost results are FPs. This removes the only two FPs on this project for this query 🎉 The pattern is:

for (f = t = l; *f; f++) // <--- use
    if (streq(*f, s))
            free(*f); // <--- free
    else
            *(t++) = *f;

We previously claimed that *f dereferenced the memory that was freed by free, but that's obviously not the case.

vim__vim

cpp/path-injection

TODO

cpp/unbounded-write

TODO

wireshark__wireshark

cpp/type-confusion

TODO

keepassxc

cpp/type-confusion

TODO

neovim__neovim

cpp/unbounded-write

TODO

Copilot wasn't able to review any files in this pull request.

Files not reviewed (6)

• cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll: Language not supported
• cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll: Language not supported
• cpp/ql/test/experimental/query-tests/Security/CWE/CWE-409/DecompressionBombs/DecompressionBombs.expected: Language not supported
• cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected: Language not supported
• cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp: Language not supported
• cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected: Language not supported

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

[jketema]

You got some more test failures. Internal PR I missed.

[jketema]

DCA shows quite a lot of alert changes. What's the deal with those?

[MathiasVP]

DCA shows quite a lot of alert changes. What's the deal with those?

I'm still reviewing some of them. So far they're all FP that are now gone 🎉 I'll continue to update the PR description as I go through the results (but as you say there are quite a lot 😅)