Skip to content
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

Crash Due to Incorrect Handling of Temporary Variable Enclosing Element #6473

Closed
iamsanjaymalakar opened this issue Mar 1, 2024 · 0 comments

Comments

@iamsanjaymalakar
Copy link
Member

Description

During the execution of resource leak inference on the NJR benchmark, the Checker Framework encountered a crash related to incorrect identification of the nearest enclosing element for temporary variable declarations. The current logic defaults to using the enclosing class instead of the nearest enclosing method, leading to inaccurate analysis and management of temporary variables. Due to this login the temporary vairables are being identified as a field, which lead to crash in the resource leak inference.

Commands to Reproduce

To reproduce the issue, you have two options:

  1. Run the AInferResourceLeakAjavaTest Suite
    Execute the AInferResourceLeakAjavaTest test suite, which includes a specific regression test designed to highlight this issue:
./gradlew AinferResourceLeakAjavaTest

This will run the suite, including the checker/tests/ainfer-resourceleak/non-annotated/CrashForTempVar.java test case, created to replicate this bug.

  1. Analyze the NJR-1 Benchmark Project
    Alternatively, analyze the url49fd521e8b_Kineolyan_JSpec_tgz-pJ8-fw_MainJ8 project within the NJR-1 benchmark, ensuring to enable whole program inference:

Inputs

Incorrect Current Implementation (in MustCallTransfer.java):

Element enclosingElement;
TreePath path = atypeFactory.getPath(tree);
if (path == null) {
  enclosingElement = TreeUtils.elementFromUse(tree).getEnclosingElement();
} else {
  ClassTree classTree = TreePathUtil.enclosingClass(path);
  enclosingElement = TreeUtils.elementFromDeclaration(classTree);
}

Proposed Correct Approach (inspired from CFGTranslationPhaseOne::findOwner):

MethodTree enclosingMethod = TreePathUtil.enclosingMethod(getCurrentPath());
if (enclosingMethod != null) {
  return TreeUtils.elementFromDeclaration(enclosingMethod);
} else {
  ClassTree enclosingClass = TreePathUtil.enclosingClass(getCurrentPath());
  return TreeUtils.elementFromDeclaration(enclosingClass);
}

The proposed change is if for a created temporary variable the enclosing element will priorotize the enclosing method first, then the enclosing class.

Output

The execution of the AInferResourceLeakAjavaTest suite, particularly the test case CrashForTempVar.java, triggers a crash in the Checker Framework. This incident illustrates the critical issue with the framework's current mechanism for handling the scope of temporary variables.

Expectation

The Checker Framework should correctly identify the nearest enclosing method as the context for temporary variables, rather than erroneously defaulting to the enclosing class. Correcting this behavior is essential for accurate analysis of temporary variables, especially in complex software systems like those encountered in the NJR benchmark. This change is expected to resolve the crash and improve the framework's reliability and accuracy in resource leak inference.

@mernst mernst closed this as completed in 7743e5d Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant