Skip to content

Fixes in cpp/global-use-before-init #19676

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mrigankpawagi
Copy link

@mrigankpawagi mrigankpawagi commented Jun 5, 2025

This PR introduces two fixes in the cpp/global-use-before-init query, which currently has two very basic inaccuracies.

Write after read

Consider the following example.

int x;

int main() {
    int woof = x;
    x = 42;
}

The query does not raise an alert on this because main does not satisfy useFunc (which holds if the function never writes to, but reads from, the global variable). The fix to useFunc modifies this condition to include functions which have a read that may not be preceded by a write inside the function.

Write before function call

Consider the following example.

int x;

void foo() {
    int meow = x;
}

int main() {
    x = 0;
    foo();
}

The query raises an alert if in the recursive step of uninitializedBefore, f never writes to v. However, what we really want is that g never writes to v (and if it does, locallyUninitialisedAt handles the case). Further, it is not clear why functionInitialises does not consider initFunc to be one of the ways for a function to initialize a variable, although it is considered by initialises. This is addressed by the fixes in uninitializedBefore and functionInitialises. This also required a corresponding change in locallyUninitialisedAt.

I am aware that some more enhancements can be made, but I am not sure how open the maintainers are to modifications in this query. I would be happy to discuss more in the comments.

@Copilot Copilot AI review requested due to automatic review settings June 5, 2025 09:43
@mrigankpawagi mrigankpawagi requested review from a team as code owners June 5, 2025 09:43
@github-actions github-actions bot added C++ Rust Pull requests that update Rust code labels Jun 5, 2025
Copilot

This comment was marked as outdated.

@mrigankpawagi mrigankpawagi reopened this Jun 5, 2025
@github-actions github-actions bot removed the Rust Pull requests that update Rust code label Jun 5, 2025
@mrigankpawagi mrigankpawagi requested a review from Copilot June 5, 2025 09:50
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 refines the global‐use‐before‐init query by distinguishing in‐function initializations that dominate a use, and by correcting which function is checked for prior writes in the recursive uninitialised‐before logic.

  • Introduce dominatingInitInFunc to detect lvalue writes that dominate a given node.
  • Update useFunc, uninitialisedBefore, functionInitialises, and locallyUninitialisedAt to use the new predicate and fix the function scope in recursive checks.
Comments suppressed due to low confidence (2)

cpp/ql/src/Critical/GlobalUseBeforeInit.ql:25

  • Add tests covering scenarios where a global variable is written in the same function both before and after a read, to validate that dominatingInitInFunc correctly distinguishes dominating writes from non-dominating ones.
predicate dominatingInitInFunc(GlobalVariable v, Function f, ControlFlowNode node) {

cpp/ql/src/Critical/GlobalUseBeforeInit.ql:39

  • [nitpick] Consider adding a code comment here to explain that this check filters out any reads dominated by a prior write in the same function, making the intent clearer for future maintainers.
    not dominatingInitInFunc(v, f, access)

@jketema jketema removed the request for review from a team June 5, 2025 12:02
@jketema
Copy link
Contributor

jketema commented Jun 5, 2025

Hi @mrigankpawagi,

Thanks for your contribution. We are always open to fixes to our queries. I would like to mention that we generally do not run this query, because it has performance issues due to its use of the points-to library. This also explains why some of the issues you're observing have been present for so long.

As part of this contribution, could you add some relevant tests to cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit? Your code snippets from above seem like a good start.

I see your changes currently do not satisfy our formatting guidelines. The easiest way to fix this is by loading the query file in VScode with the CodeQL extension installed, and asking VScode to format the file.

@mrigankpawagi
Copy link
Author

mrigankpawagi commented Jun 5, 2025

Thanks for your response, @jketema! I have addressed your comments.

@jketema jketema self-requested a review June 6, 2025 15:53
@jketema jketema self-assigned this Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants