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.

Sign up for GitHub

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

Jump to bottom

Owning fields should not be compared with string containment #6280

Merged
merged 8 commits into from
Nov 3, 2023
Merged

Owning fields should not be compared with string containment #6280

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0bfdaf8
failing test
kelloggm Nov 1, 2023
7becc3c
add issue URL
kelloggm Nov 1, 2023
caedc6c
Merge branch 'master' of https://github.com/typetools/checker-framewo…
kelloggm Nov 1, 2023
d997555
port Calvin's extraction of the relevant code into a method
kelloggm Nov 1, 2023
9860ce4
fix bug
kelloggm Nov 1, 2023
d01ffc6
add a note
kelloggm Nov 1, 2023
41fb766
Merge branch 'master' of https://github.com/typetools/checker-framewo…
kelloggm Nov 1, 2023
c12e758
test case
kelloggm Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.Modifier;
import javax.lang.model.element.TypeElement;
import javax.lang.model.element.VariableElement;
import org.checkerframework.checker.calledmethods.CalledMethodsVisitor;
import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.CreatesMustCallForToJavaExpression;
Expand All @@ -20,10 +21,13 @@
import org.checkerframework.checker.mustcall.qual.NotOwning;
import org.checkerframework.checker.mustcall.qual.Owning;
import org.checkerframework.common.basetype.BaseTypeChecker;
import org.checkerframework.dataflow.expression.FieldAccess;
import org.checkerframework.dataflow.expression.JavaExpression;
import org.checkerframework.dataflow.qual.Pure;
import org.checkerframework.framework.flow.CFAbstractStore;
import org.checkerframework.framework.flow.CFAbstractValue;
import org.checkerframework.framework.util.JavaExpressionParseUtil;
import org.checkerframework.framework.util.StringToJavaExpression;
import org.checkerframework.javacutil.AnnotationMirrorSet;
import org.checkerframework.javacutil.AnnotationUtils;
import org.checkerframework.javacutil.ElementUtils;
Expand Down Expand Up @@ -355,7 +359,7 @@ private static AnnotationMirrorSet getEnsuresCalledMethodsAnnotations(

@Override
public Void visitVariable(VariableTree tree, Void p) {
Element varElement = TreeUtils.elementFromDeclaration(tree);
VariableElement varElement = TreeUtils.elementFromDeclaration(tree);

if (varElement.getKind().isField()
&& !noLightweightOwnership
Expand All @@ -375,7 +379,7 @@ public Void visitVariable(VariableTree tree, Void p) {
*
* @param field the declaration of the field to check
*/
private void checkOwningField(Element field) {
private void checkOwningField(VariableElement field) {

if (checker.shouldSkipUses(field)) {
return;
Expand Down Expand Up @@ -429,7 +433,7 @@ private void checkOwningField(Element field) {
rlTypeFactory.ensuresCalledMethodsValueElement,
String.class);
for (String value : values) {
if (value.contains(field.getSimpleName().toString())) {
if (expressionEqualsField(value, field)) {
List<String> methods =
AnnotationUtils.getElementValueArray(
ensuresCalledMethodsAnno,
Expand Down Expand Up @@ -467,6 +471,23 @@ private void checkOwningField(Element field) {
}
}

/**
* Determine if the given expression <code>e</code> refers to <code>this.field</code>.
*
* @param e the expression
* @param field the field
* @return true if <code>e</code> refers to <code>this.field</code>
*/
private boolean expressionEqualsField(String e, VariableElement field) {
try {
JavaExpression je = StringToJavaExpression.atFieldDecl(e, field, this.checker);
return je instanceof FieldAccess && ((FieldAccess) je).getField().equals(field);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to check if the receiver is this?

Copy link
Contributor Author

@kelloggm kelloggm Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

StringToJavaExpression should handle that for us: it uses the supplied location (field, the declaration of the owning field, in this case) and the given string to find the appropriate program element, and associates that with the result.

A potential point of confusion in my implementation is that field is used for two purposes:

  • it's the element corresponding to the owning field, so the object that StringToJavaExpression converts the input string into is compared against it
  • it is used as the location at which StringToJavaExpression should consider the string e to occur in the code: that is, the context

Copy link
Contributor

Choose a reason for hiding this comment

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

Could I write something like

@MustCall("close")
class SneakyDestructor {
  private final @Owning Closeable resource;

  // ...

  @EnsuresCalledMethods(value = "#1.resource", methods = "close")
  void close(SneakyDestructor other) {
    other.close();
  }
}

?

In that case, the field referred to by the expression really is SneakyDestructor.resource, but the destructor clearly doesn't close its own @Owning field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you mean now, thanks. I fleshed out your test case and added it to this PR, and a bit to my surprise the tests are passing.

} catch (JavaExpressionParseUtil.JavaExpressionParseException ex) {
// The parsing error will be reported elsewhere, assuming e was derived from an annotation.
return false;
}
}

/**
* Checks if WPI is enabled for the Resource Leak Checker inference. See {@link
* ResourceLeakChecker#ENABLE_WPI_FOR_RLC}.
Expand Down
32 changes: 32 additions & 0 deletions checker/tests/resourceleak/OwningFieldStringComparison.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Test case for https://github.com/typetools/checker-framework/issues/6276

import java.net.Socket;
import org.checkerframework.checker.calledmethods.qual.*;
import org.checkerframework.checker.mustcall.qual.*;

@InheritableMustCall("a")
public class OwningFieldStringComparison {

// :: error: required.method.not.called
@Owning Socket s;

// important to the bug: the name of this field must contain
// the name of the owning socket
/* @NotOwning */ Socket s2;

// Note this "destructor" closes the wrong socket
@EnsuresCalledMethods(value = "this.s2", methods = "close")
public void a() {
try {
this.s2.close();
} catch (Exception e) {

} finally {
try {
this.s2.close();
} catch (Exception e) {

}
}
}
}