Skip to content

C++: Mark the write to fprintf's 0'th argument as partial #20242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 19, 2025

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Aug 18, 2025

This fixes a FN that @jketema noticed on the Coding Standards queries.

The new results in cpp/non-constant-format and cpp/tainted-format-string highlight some really weird code:

#define die(fmt,args...) do { fprintf(stderr, fmt, ## args ); fflush(stderr); exit(-1); } while(0)
...
if(/*NEEDTMP &&*/ !(tmp = (unsigned char*)malloc(ns))) die(stderr, "malloc error\n");

We get new results on stderr. It took me a second to spot the issue, but it looks like die expands to:

do { fprintf(stderr, stderr, "malloc error\n"); fflush(stderr); exit(-1); } 

Which seems very bad! So this looks like TPs 🎉

@github-actions github-actions bot added the C++ label Aug 18, 2025
@MathiasVP MathiasVP changed the title C++: Mark the write to fprintf's 0'th argument as a partial write C++: Mark the write to fprintf's 0'th argument as patial Aug 18, 2025
@MathiasVP MathiasVP marked this pull request as ready for review August 19, 2025 10:05
@MathiasVP MathiasVP requested a review from a team as a code owner August 19, 2025 10:05
@Copilot Copilot AI review requested due to automatic review settings August 19, 2025 10:05
Copy link
Contributor

@Copilot Copilot AI left a comment

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 improves taint tracking for formatting functions like fprintf by marking writes to their FILE stream parameter as partial writes. This ensures that taint flows through the stream parameter correctly when it's used as both input and output.

  • Adds a new test case demonstrating taint flow through fprintf's FILE parameter
  • Implements isPartialWrite method in the FormattingFunction class to mark stream outputs as partial
  • Updates the SSA implementation to properly handle the partial flow logic

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
FormattingFunction.qll Implements isPartialWrite to mark stream parameters as partial writes
SsaImpl.qll Fixes variable reference in modeledFlowBarrier predicate
taint.cpp Adds test case for taint flow through fprintf FILE parameter
taint.ql Adds support for indirect_sink in test queries
*.expected Updated test expectations reflecting the new taint flow behavior

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@MathiasVP MathiasVP changed the title C++: Mark the write to fprintf's 0'th argument as patial C++: Mark the write to fprintf's 0'th argument as partial Aug 19, 2025
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Aug 19, 2025

final override predicate isPartialWrite(FunctionOutput output) {
exists(int outputParameterIndex |
output.isParameterDeref(outputParameterIndex) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Given where you added this, this presumably also leads more flow on the first argument of functions like sprintf. Is that correct? If so, is that what we want?

Copy link
Contributor Author

@MathiasVP MathiasVP Aug 19, 2025

Choose a reason for hiding this comment

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

The this.getOutputParameterIndex(true) conjunct only holds for formatting functions which specify that the output parameter is a stream, and sprintf implements this predicate with isStream = false. So this shouldn't affect sprintf.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification!

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM

@MathiasVP MathiasVP merged commit ea8d766 into github:main Aug 19, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants