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:

A note on cpp/type-confusion

I investigated the result changes on Wireshark, where the number of results drop from 209 to 14. As far as I can see, the results disappear because we use asExpr as the definition of the sink, and for all the disappeared results it's really the indirect expression that reaches the sink, and not the value of actual expression. So I think these results disappearing is good!

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. I think these results are actually TPs, but they require us to track 3 levels of indirections on void pointers, and we currently limit this to 2.

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

The removed results are all due to pointer/pointee conflation. These paths are extremely hard to follow, so they may actually be TPs in the end 🤷. But I can certainly see the conflation in the flow path.

cpp/unbounded-write

The removed results are all due to pointer/pointee conflation. Same comment as for cpp/path-injection applies.

wireshark__wireshark

cpp/type-confusion

See the note above.

keepassxc

cpp/type-confusion

See the note above.

neovim__neovim

cpp/unbounded-write

Also caused by pointer/pointee conflation.

[Copilot]

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 😅)

[jketema]

LGTM