facebook / facebook/infer

[Java] False positive for null dereference

Open
#1,644 0 comments 0 reactions 0 assignees View on GitHub
false-positive java
Dominant language
OCaml
Stars
15.7k
Forks
2.1k
Avg merge
19h 36m
Merged PRs (30d)
13

Description

## Information
- [x] The version of infer from `infer --version`: 1.1.0
- [x] Your operating system and version: using https://github.com/facebook/infer/blob/main/docker/1.1.0/Dockerfile
- [x] Which command you ran, for example `infer run --fail-on-issue -- mvn compile`.
- [/] The full output in a paste, for instance a [gist](https://gist.github.com/).
- [ ] If possible, a minimal example to reproduce your problem (for instance, some code where
infer reports incorrectly, together with the way you run infer to reproduce the incorrect
report).

## Logs

Unfortunately, I am unable to create a smaller reproducer so far (and the code where infer finds this is not shareable), but the logs are pretty clear:

```java
source.java:789: error: Null Dereference
object `inProgress` last assigned on line 788 could be null and is dereferenced by call to `updateInProgressStatus(...)` at line 789.
787. final boolean inProgress =
788. task.getStatus() != null && task.getStatus().equals(Task.Status.IN_PROGRESS);
789. > updateInProgressStatus(connection, task, inProgress);
790. }
```

So, infer thinks that `inProgress` can be null which is not possible for 2 reasons:
- code compilation was fine
- it's a primitive type `boolean`
- it is the result of a `a && b` which cannot return null (it's a Java language comparison, always returning a `boolean`)
- `updateInProgressStatus` takes `(A, B, boolean)` as input parameter types (and so not a `Boolean` but a `boolean`)

So this is fairly weird.

I think that Infer should know better that a `&&` comparison must return a `boolean`, but also that `boolean` types (or any primitive type, cannot be null, by definition). It's a good invariant to check for I think.

## Test 1: rewrite the code with an if

If I try to change the code a bit, then infer is happy, even though it's the exact same behaviour:

```java
final boolean inProgress;
if (task.getStatus() != null && task.getStatus().equals(Task.Status.IN_PROGRESS)) {
inProgress = true;
} else {
inProgress = false;
}
updateInProgressStatus(connection, task, inProgress);
```

Infer still complains about it:

```
source.java:793: error: Null Dereference
#14 410.1 object `inProgress` last assigned on line 791 could be null and is dereferenced by call to `updateInProgressStatus(...)` at line 793.
#14 410.1 791. inProgress = false;
#14 410.1 792. }
#14 410.1 793. > updateInProgressStatus(connection, task, inProgress);
#14 410.1 794. }
#14 410.1 795.
```

## Test 2: Test 1 but with inverted if condition

I was curious to see if inverting the if condition would help infer think it's correct:

```java
final boolean inProgress;
if (task.getStatus() == null || !task.getStatus().equals(Task.Status.IN_PROGRESS)) {
inProgress = false;
} else {
inProgress = true;
}
updateInProgressStatus(connection, task, inProgress);
```

But infer still thinks that this isn't correct:

```
object `inProgress` last assigned on line 789 could be null and is dereferenced by call to `updateInProgressStatus(...)` at line 793.
#15 422.4 791. inProgress = true;
#15 422.4 792. }
#15 422.4 793. > updateInProgressStatus(connection, task, inProgress);
#15 422.4 794. }
```

## Test 3: yet another form

This time by assigning the variable first, and overriding it instead:

```java
boolean inProgress = false;
if (task.getStatus() != null && task.getStatus().equals(Task.Status.IN_PROGRESS)) {
inProgress = true;
}
updateInProgressStatus(connection, task, inProgress);
```

But, still the same:

```
#15 423.7 source.java:791: error: Null Dereference
#15 423.7 object `inProgress` last assigned on line 787 could be null and is dereferenced by call to `updateInProgressStatus(...)` at line 791.
#15 423.7 789. inProgress = true;
#15 423.7 790. }
#15 423.7 791. > updateInProgressStatus(connection, task, inProgress);
#15 423.7 792. }
```

## Test 4

This time, I remove the if assignment (but then it's no longer strictly equivalent):

```java
boolean inProgress = false;
updateInProgressStatus(connection, task, inProgress);
```

Still, the same:

```java
#15 433.3 source.java:788: error: Null Dereference
#15 433.3 object `inProgress` last assigned on line 787 could be null and is dereferenced by call to `updateInProgressStatus(...)` at line 788.
#15 433.3 787. boolean inProgress = false;
#15 433.3 788. > updateInProgressStatus(connection, task, inProgress);
#15 433.3 789. }
#15 433.3 790.
```

## Test 5

I move that inProgress variable set inside updateInProgressStatus method (first line in the method), and now infer is happy (finally).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.