-
Notifications
You must be signed in to change notification settings - Fork 69
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 NilAway FPs on channel sends/receives #193
Conversation
Golden TestWarning ❌ NilAway errors reported on stdlib are different 📉. 3043 errors on base branch (main, 9b1d75d) Diffs- /opt/hostedtoolcache/go/1.21.6/x64/src/context/context_test.go:124:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - context/context_test.go:124:9: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `WithValue(...)` to `precanceledChild` at context/context_test.go:122:2
-
- (Same nil source could also cause potential nil panic(s) at 1 other place(s): "context/context_test.go:124:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/context/context_test.go:124:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:582:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `withoutCancelCtx.Done()`)
- - context/context_test.go:124:9: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `WithValue(...)` to `precanceledChild` at context/context_test.go:122:2
- /opt/hostedtoolcache/go/1.21.6/x64/src/crypto/internal/randutil/randutil.go:34:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - randutil/randutil.go:16:2: nilable value assigned into global variable `closedChan`
- - randutil/randutil.go:34:9: global variable `closedChan` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/crypto/tls/handshake_server_test.go:2050:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - tls/handshake_server_test.go:2050:9: result 0 of `Done()` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/crypto/tls/quic.go:417:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - tls/conn.go:1510:20: result 0 of `Done()` assigned into field `cancelc` via the assignment(s):
- - `context.WithCancel(ctx)` to `handshakeCtx` at tls/conn.go:1503:2
- - tls/quic.go:417:9: field `cancelc` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 1 other place(s): "tls/quic.go:407:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/database/sql/ctxutil.go:21:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - sql/ctxutil.go:21:10: result 0 of `Done()` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 13 other place(s): "sql/ctxutil.go:40:9", "sql/ctxutil.go:57:9", "sql/ctxutil.go:74:9", "sql/ctxutil.go:91:9", "sql/sql.go:1219:10", "sql/sql.go:1336:10", "sql/sql.go:1290:9", "sql/sql.go:2174:4", "sql/sql.go:2210:9", "sql/sql.go:2255:9", "sql/sql.go:2966:9", "sql/sql.go:2963:9", and "sql/sql.go:2969:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/database/sql/ctxutil.go:21:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - sql/ctxutil.go:21:10: result 0 of `Done()` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 17 other place(s): "sql/ctxutil.go:40:9", "sql/ctxutil.go:57:9", "sql/ctxutil.go:74:9", "sql/ctxutil.go:91:9", "sql/sql.go:1219:10", "sql/sql.go:1336:10", "sql/sql.go:1290:9", "sql/sql.go:2174:4", "sql/sql.go:2210:9", "sql/sql.go:2255:9", "sql/sql.go:2966:9", "sql/sql.go:2963:9", "sql/sql.go:2969:9", "sql/fakedb_test.go:692:11", "sql/fakedb_test.go:799:9", "sql/sql_test.go:345:9", and "sql/sql_test.go:3031:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/database/sql/fakedb_test.go:287:3: Potential nil panic detected. Observed nil flow from source to dereference point:
- - sql/fakedb_test.go:290:17: literal `nil` assigned into field `waitingCh`
- - sql/fakedb_test.go:287:3: field `waitingCh` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/database/sql/sql.go:1063:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - sql/sql.go:1070:19: literal `nil` assigned into field `cleanerCh`
- - sql/sql.go:1063:10: field `cleanerCh` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/database/sql/sql.go:2966:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - sql/sql.go:2966:9: unassigned variable `txctxDone` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/internal/fuzz/fuzz.go:225:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - fuzz/fuzz.go:225:10: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `context.WithTimeout(...)` to `ctx` at fuzz/fuzz.go:107:3,
- - `ctx.Done()` to `doneC` at fuzz/fuzz.go:114:2
-
- (Same nil source could also cause potential nil panic(s) at 3 other place(s): "fuzz/worker.go:111:10", "fuzz/worker.go:776:10", and "fuzz/worker.go:1190:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/internal/fuzz/fuzz.go:384:8: Potential nil panic detected. Observed nil flow from source to dereference point:
- - fuzz/fuzz.go:384:8: unassigned variable `inputC` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/internal/fuzz/fuzz.go:388:8: Potential nil panic detected. Observed nil flow from source to dereference point:
- - fuzz/fuzz.go:388:8: unassigned variable `minimizeC` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/internal/fuzz/worker.go:119:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - fuzz/worker.go:343:12: literal `nil` assigned into field `termC`
- - fuzz/worker.go:119:10: field `termC` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/log/slog/logger_test.go:343:4: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - slog/logger_test.go:343:4: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `context.WithTimeout(...)` to `ctx` at slog/logger_test.go:333:2
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/dial.go:592:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - net/dial.go:592:10: result 0 of `Done()` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 4 other place(s): "net/fd_unix.go:143:11", "net/fd_unix.go:63:10", "net/lookup.go:277:9", and "net/lookup.go:333:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/dial.go:592:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - net/dial.go:592:10: result 0 of `Done()` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 6 other place(s): "net/fd_unix.go:143:11", "net/fd_unix.go:63:10", "net/lookup.go:277:9", "net/lookup.go:333:9", "net/dial_test.go:152:5", and "net/dial_unix_test.go:100:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/h2_bundle.go:7666:12: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - http/h2_bundle.go:7666:12: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `http2shouldRetryRequest(...)` to `req` at http/h2_bundle.go:7652:7
-
- (Same nil source could also cause potential nil panic(s) at 17 other place(s): "http/h2_bundle.go:8164:9", "http/h2_bundle.go:8396:10", "http/h2_bundle.go:8559:10", "http/h2_bundle.go:8513:11", "http/h2_bundle.go:8440:9", "http/h2_bundle.go:8578:9", "http/h2_bundle.go:8920:10", "http/h2_bundle.go:9696:9", "http/h2_bundle.go:10040:9", "http/server.go:2816:10", "http/server.go:3415:9", "http/transport.go:573:10", "http/transport.go:1407:11", "http/transport.go:1421:9", "http/transport.go:1727:10", "http/transport.go:2252:10", and "http/transport.go:2697:10".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/h2_bundle.go:8552:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/h2_bundle.go:8552:10: literal `nil` uninitialized; nil channel accessed via the assignment(s):
- - `nil` to `respHeaderTimer` at http/h2_bundle.go:8556:4
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/h2_bundle.go:8552:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/h2_bundle.go:8552:10: unassigned variable `respHeaderTimer` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/h2_bundle.go:8554:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/h2_bundle.go:8554:10: literal `nil` uninitialized; nil channel accessed via the assignment(s):
- - `nil` to `respHeaderRecv` at http/h2_bundle.go:8555:4
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/h2_bundle.go:8554:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/h2_bundle.go:8554:10: unassigned variable `respHeaderRecv` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/pprof/pprof.go:113:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - pprof/pprof.go:113:9: result 0 of `Done()` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 1 other place(s): "pprof/pprof.go:300:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/serve_test.go:4788:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - http/serve_test.go:4788:9: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `<-ctxc` to `ctx` at http/serve_test.go:4786:2
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/transport.go:1405:11: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - http/client_test.go:1332:15: result 0 of `Done()` assigned into field `Cancel` via the assignment(s):
- - `context.WithCancel(...)` to `ctx` at http/client_test.go:1322:2
- - http/transport.go:1405:11: field `Cancel` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/transport.go:2669:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/transport.go:2669:10: literal `nil` uninitialized; nil channel accessed via the assignment(s):
- - `nil` to `pcClosed` at http/transport.go:2670:4
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/transport.go:2677:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/transport.go:2677:10: unassigned variable `respHeaderTimer` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/transport.go:2694:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/transport.go:2694:10: literal `nil` uninitialized; nil channel accessed via the assignment(s):
- - `nil` to `cancelChan` at http/transport.go:2696:4
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/http/transport.go:2697:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - http/transport.go:2697:10: literal `nil` uninitialized; nil channel accessed via the assignment(s):
- - `nil` to `ctxDoneChan` at http/transport.go:2700:4
- /opt/hostedtoolcache/go/1.21.6/x64/src/net/net.go:688:2: Potential nil panic detected. Observed nil flow from source to dereference point:
- - net/net.go:680:5: nilable value assigned into global variable `threadLimit`
- - net/net.go:688:2: global variable `threadLimit` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/os/exec/exec.go:664:10: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - exec/exec.go:664:10: result 0 of `Done()` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 1 other place(s): "exec/exec.go:760:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/os/signal/example_unix_test.go:40:9: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - signal/example_unix_test.go:40:9: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `signal.NotifyContext(...)` to `ctx` at signal/example_unix_test.go:21:2
- /opt/hostedtoolcache/go/1.21.6/x64/src/os/signal/signal_test.go:728:5: Potential nil panic detected. Observed nil flow from source to dereference point:
- - context/context.go:184:9: literal `nil` returned from `Done()` in position 0
- - context/context.go:105:2: returned as result 0 from interface method `Context.Done()` (implemented by `emptyCtx.Done()`)
- - signal/signal_test.go:728:5: result 0 of `Done()` uninitialized; nil channel accessed via the assignment(s):
- - `NotifyContext(...)` to `ctx` at signal/signal_test.go:715:3
-
- (Same nil source could also cause potential nil panic(s) at 4 other place(s): "signal/signal_test.go:801:9", "signal/signal_test.go:822:9", "signal/signal_test.go:844:9", and "signal/signal_test.go:872:9".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/reflect/all_test.go:1701:3: Potential nil panic detected. Observed nil flow from source to dereference point:
- - reflect/all_test.go:1701:3: unassigned variable `c` uninitialized; nil channel accessed
-
- (Same nil source could also cause potential nil panic(s) at 7 other place(s): "reflect/all_test.go:1695:13", "reflect/all_test.go:1690:16", "reflect/all_test.go:1680:10", "reflect/all_test.go:1678:3", "reflect/all_test.go:1669:3", "reflect/all_test.go:1657:3", and "reflect/all_test.go:1652:13".)
- /opt/hostedtoolcache/go/1.21.6/x64/src/runtime/abi_test.go:27:2: Potential nil panic detected. Observed nil flow from source to dereference point:
- - runtime/abi_test.go:23:5: nilable value assigned into global variable `regConfirmRun`
- - runtime/abi_test.go:27:2: global variable `regConfirmRun` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/runtime/cgocall.go:306:5: Potential nil panic detected. Observed nil flow from source to dereference point:
- - runtime/proc.go:129:5: nilable value assigned into global variable `main_init_done`
- - runtime/cgocall.go:306:5: global variable `main_init_done` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/runtime/signal_unix.go:202:3: Potential nil panic detected. Observed nil flow from source to dereference point:
- - runtime/signal_unix.go:94:2: nilable value assigned into global variable `enableSigChan`
- - runtime/signal_unix.go:202:3: global variable `enableSigChan` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/runtime/signal_unix.go:203:5: Potential nil panic detected. Observed nil flow from source to dereference point:
- - runtime/signal_unix.go:95:2: nilable value assigned into global variable `maskUpdatedChan`
- - runtime/signal_unix.go:203:5: global variable `maskUpdatedChan` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/runtime/signal_unix.go:227:3: Potential nil panic detected. Observed nil flow from source to dereference point:
- - runtime/signal_unix.go:93:2: nilable value assigned into global variable `disableSigChan`
- - runtime/signal_unix.go:227:3: global variable `disableSigChan` uninitialized; nil channel accessed
- /opt/hostedtoolcache/go/1.21.6/x64/src/time/example_test.go:156:14: Potential nil panic detected. Observed nil flow from source to dereference point:
- - time/example_test.go:150:5: nilable value assigned into global variable `c`
- - time/example_test.go:156:14: global variable `c` uninitialized; nil channel accessed |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
- Coverage 88.33% 88.31% -0.03%
==========================================
Files 55 55
Lines 9364 9344 -20
==========================================
- Hits 8272 8252 -20
Misses 917 917
Partials 175 175 ☔ View full report in Codecov by Sentry. |
d13189e
to
e0b74ef
Compare
25652b0
to
d6d74bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed internally, we did agree upon this strategy to address false positives. LGTM!
} | ||
|
||
// nilable(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the PR, I guess, but a welcome change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :)
Co-authored-by: Sonal Mahajan <[email protected]>
Co-authored-by: Sonal Mahajan <[email protected]>
1529801
to
fe6b551
Compare
See issue #192 for a more detailed discussion on handling channels in NilAway. To make this PR more self-contained, below is a partial copy of the discussion there:
See the following behaviors regarding nil / closed channels (taken from this SO post, slightly modified):
Only case 3 (sending to a closed channel) will result in a panic. The other cases arguably may not be within scope of NilAway's analysis. Moreover, NilAway currently only tracks nilabilities of channels, but not the states of them, making it unable to really handle case 2 and 4.
Since this introduces a lot more FPs than TPs (if any), this PR removes logic to require sending to/receiving from nonnil channels. We leave properly handling channels as future work (especially around tracking states of channels).
The test cases have been updated to reflect this. Specifically, we kept the test cases there to now serve as negative cases in case for future development of such a support.
Fixes #98
Fixes #167