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

Fix false positive for a corner case of short-circuiting expressions #92

Closed
yuxincs opened this issue Nov 2, 2023 · 3 comments · Fixed by #227
Closed

Fix false positive for a corner case of short-circuiting expressions #92

yuxincs opened this issue Nov 2, 2023 · 3 comments · Fixed by #227
Labels
bug Something isn't working false positive Requires more analysis and support good first issue Good for newcomers

Comments

@yuxincs
Copy link
Contributor

yuxincs commented Nov 2, 2023

NilAway is complaining about the latter dereference in the short-circuiting expressions:

func foo() bool {
  return s == nil || *s == ""
}

We have support for short-circuiting expressions in NilAway, but this is a corner case that we somehow missed. We should add support for it.

@daboross
Copy link

daboross commented Nov 17, 2023

Same here, for

type AType struct {
	Field int
}

func F1(a *AType, b *AType) bool {
	return a != nil && b != nil && a.Field != b.Field
}

func TestF1(t *testing.T) {
	if F1(nil, nil) {
		t.Fatalf("f1 should not accept nil, nil")
	}
}

@mavirn
Copy link

mavirn commented Nov 17, 2023

Similar experience here for this code block where short circuiting will prevent accessing a field of a nil item.

func (r *Room) getInfoAboutRoom() {
    hasField := r.Obj != nil && r.Obj.Field != nil
    ...

@davecgh
Copy link

davecgh commented Nov 21, 2023

It also is giving false positives for short circuiting slice indexing behind a length check which is a very common and convenient operation because it returns 0 for both nil slices as well as non-nil empty slices meaning only a single check is necessary to cover both cases.

Here is a simple program to reproduce the false positive:

package main

import "fmt"

func isFirstByteZero(s []byte) bool {
        return len(s) > 0 && s[0] == 0
}

func main() {
        fmt.Println(isFirstByteZero(nil))
}
$ nilaway ./...
nilawayfp\main.go:6:23: error: Potential nil panic detected. Observed nil flow from source to dereference point:
        -> nilawayfp\main.go:10:30: literal `nil` passed as arg `s` to `isFirstByteZero()`
        -> nilawayfp\main.go:6:23: function parameter `s` sliced into

@sonalmahajan15 sonalmahajan15 added good first issue Good for newcomers false positive Requires more analysis and support labels Dec 5, 2023
@sonalmahajan15 sonalmahajan15 added the bug Something isn't working label Dec 7, 2023
sonalmahajan15 added a commit that referenced this issue Mar 26, 2024
This PR revisits the non-conditional short-circuit handling. Following
are the changes:

- The main change is to repurpose the `AddNilCheck` function to trigger
the handling, instead of the earlier custom (and duplicated)
implementation in `asNilCheckExpr`.
- `AddNilCheck` is strengthened to also be able to handle negations.
E.g., `!(x == nil)`
- The added benefit of using `AddNilCheck()` is also that it already has
support for `len()` checks, which the non-conditional handling can
leverage right away. A new extensive set of test cases for len checks
(as pointed in issue #92) is also added.
sonalmahajan15 added a commit that referenced this issue Apr 3, 2024
This PR adds support for short-circuit `||` in non-conditional flows,
thereby reducing false positives. For example,
```
return x == nil || *x == 1
```
was reported as a false positive, since NilAway only analyzed `&&`.

We apply logic similar to the handling of `&&`. Because this analysis
resides in the recursion of the short-circuit expression, where we have
limited context visibility, it makes it difficult to accurately analyze
complex expressions. Therefore, with extending support for `||`, we had
to trade-off some of the precision we previously had with only the `&&`
support. However, we made this tough choice since empirically we
observed that complex cases that we had to trade-off did not occur
frequently enough, while simple `||` patterns were more prevalent. I
have created issue #226 to keep a track of the comprehensive support
that we plan to add in the future.

[closes #92 ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working false positive Requires more analysis and support good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants