Skip to content

Shared/C++: Handle non-standard return values in MaD flow sources/sinks - #19569

Merged
MathiasVP merged 4 commits into
github:mainfrom
MathiasVP:fix-return-value-surces-in-mad
May 23, 2025
Merged

Shared/C++: Handle non-standard return values in MaD flow sources/sinks#19569
MathiasVP merged 4 commits into
github:mainfrom
MathiasVP:fix-return-value-surces-in-mad

Conversation

@MathiasVP

Copy link
Copy Markdown
Contributor

In #19563@jketema is adding flow sources for (among other things) GetCommandLineA which should have the following MaD specification:

["", "", False, "GetCommandLineA", "", "", "ReturnValue[*]", "local", "manual"]

(because it's not the pointer returned by GetCommandLineA that's user controlled - it's the data that's pointed to!)

However, we noticed that this is not parsed correctly by the current implementation of SourceSinkInterpretation::interpretOutput since it only has a case for getStandardReturnValueKind (and ReturnValue is the standard return value kind, not ReturnValue[*]).

This PR fixes that missing case, and I've checked that this makes the MaD specifications we want to have in #19563 work 🎉

…Output' and 'interpretInput' to handle non-standard return value input/output. This is needed to support C++'s ReturnValue[**] notation.
CopilotAI review requested due to automatic review settings May 23, 2025 10:25
@MathiasVP
MathiasVP requested a review from a team as a code ownerMay 23, 2025 10:25
@MathiasVPMathiasVP added the no-change-note-required This PR does not need a change note label May 23, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extends the MaD flow specification to correctly handle non-standard return-value indirections (e.g. ReturnValue[*]) by delegating to a new getReturnValueKind helper in both the QL and C++ FlowSummary implementations.
Key changes:

  • Introduce getReturnValueKind(string) and update interpretOutput/interpretInput to use it when ReturnValue has an argument.
  • Delegate getStandardReturnValueKind() to getReturnValueKind("") in both QL and C++ modules.
  • Update model-as-data tests to expect indirect return-value flows (fix missing $ ir marks).

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

FileDescription
shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qllAdd getReturnValueKind, extend interpretOutput/interpretInput to handle starred return kinds.
cpp/ql/test/library-tests/dataflow/models-as-data/tests.cppUpdate test expectations for indirect return-value sources.
cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qllRoute getStandardReturnValueKind() through new getReturnValueKind.

Comment threadshared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll
Comment threadshared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll

@jketemajketema left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment, otherwise LGTM. Does this need a C++ DCA experiment?

Comment threadshared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated
@MathiasVP

Copy link
Copy Markdown
ContributorAuthor

One small comment, otherwise LGTM. Does this need a C++ DCA experiment?

Thanks! Yeah, I've started a DCA run now. I don't think we have any MaD specifications that are currently affected by this, but it doesn't hurt to double check

@jketemajketema left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@MathiasVP

Copy link
Copy Markdown
ContributorAuthor

DCA was uneventful (as expected)

@MathiasVP
MathiasVP merged commit 0822ded into github:mainMay 23, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++DataFlow Libraryno-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@MathiasVP@jketema