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

Fix nil pointer dereference #4884

Merged
merged 2 commits into from Aug 21, 2023
Merged

Fix nil pointer dereference #4884

merged 2 commits into from Aug 21, 2023

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Aug 21, 2023 •
edited

Motivation

Latest release causes a nil pointer dereference error for some users:

2023-08-21T19:48:11.170+0800	INFO	grpc.Smesher	GRPC SmesherService.PostSetupStatus	{"module": "grpc"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x1 addr=0x0 pc=0x7ff6c6731597]

goroutine 27356 [running]:
github.com/spacemeshos/go-spacemesh/api/grpcserver.statusToPbStatus(...)
	D:/a/go-spacemesh/go-spacemesh/api/grpcserver/smesher_service.go:261
github.com/spacemeshos/go-spacemesh/api/grpcserver.SmesherService.PostSetupStatus({{0x7ff6c75ea7e8, 0xc0148d8510}, {0x7ff6c75dff30, 0xc008a7c800}, {0x7ff6c75e7370, 0xc000626540}, 0x3b9aca00, {{0xc0006b3008, 0x14}, 0x1c, ...}}, ...)
	D:/a/go-spacemesh/go-spacemesh/api/grpcserver/smesher_service.go:180 +0x97
github.com/spacemeshos/api/release/go/spacemesh/v1._SmesherService_PostSetupStatus_Handler.func1({0x7ff6c75def38, 0xc001ae2ea0}, {0x7ff6c6f7d200?, 0xc001ae2d50})
	C:/Users/runneradmin/go/pkg/mod/github.com/spacemeshos/api/release/go@v1.19.0/spacemesh/v1/smesher.pb.go:673 +0x78
github.com/grpc-ecosystem/go-grpc-middleware/logging/zap.UnaryServerInterceptor.func1({0x7ff6c75def38, 0xc001ae2db0}, {0x7ff6c6f7d200, 0xc001ae2d50}, 0xc000514ee0, 0xc003ad7800)
	C:/Users/runneradmin/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.4.0/logging/zap/server_interceptors.go:31 +0x115
google.golang.org/grpc.getChainUnaryHandler.func1({0x7ff6c75def38, 0xc001ae2db0}, {0x7ff6c6f7d200, 0xc001ae2d50})
	C:/Users/runneradmin/go/pkg/mod/google.golang.org/grpc@v1.57.0/server.go:1179 +0xb9
github.com/grpc-ecosystem/go-grpc-middleware/tags.UnaryServerInterceptor.func1({0x7ff6c75def38?, 0xc001ae2cf0?}, {0x7ff6c6f7d200, 0xc001ae2d50}, 0xc000514ee0, 0xc014a5d3c0)
	C:/Users/runneradmin/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.4.0/tags/interceptors.go:23 +0xa6
google.golang.org/grpc.chainUnaryInterceptors.func1({0x7ff6c75def38, 0xc001ae2cf0}, {0x7ff6c6f7d200, 0xc001ae2d50}, 0xc006235a58?, 0x7ff6c6ecd660?)
	C:/Users/runneradmin/go/pkg/mod/google.golang.org/grpc@v1.57.0/server.go:1170 +0x8f
github.com/spacemeshos/api/release/go/spacemesh/v1._SmesherService_PostSetupStatus_Handler({0x7ff6c70c1f00?, 0xc000278a20}, {0x7ff6c75def38, 0xc001ae2cf0}, 0xc0065aae70, 0xc005512820)
	C:/Users/runneradmin/go/pkg/mod/github.com/spacemeshos/api/release/go@v1.19.0/spacemesh/v1/smesher.pb.gopanic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x1 addr=0x0 pc=0x7ff6c6731597]

Changes

  • ensure ProviderID is initialized before dereferencing

Test Plan

  • new tests were added that failed the same way as reported by affected users and pass with the change.

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed

DevOps Notes

  • This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
  • This PR does not affect public APIs
  • This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
  • This PR does not make changes to log messages (which monitoring infrastructure may rely on)

@piersy
Add peer info api (#4845)
64bccb6 
## Motivation
Closes #4778

Note that this is in a draft state because it needs spacemeshos/api#258 to be merged and I guess a release of the API package, before it can be merged. I plan to remove the replace directive for the api in the mod file when, the api has been released.

## Changes
Adds a new streaming API endpoint to the node service to return info about connected peers.

Note this PR changes the PostProviderID field to be exported to allow for config to be easily serialised/deserialised in order to perform a deep copy.

## Test Plan
Added a test, the test doesn't check whether tags are since that part would make things a bit trickier. However through manual checking I can see that tags are being assigned.

```shell
$ grpc_cli call  localhost:9092 spacemesh.v1.NodeService.PeerInfo ''
connecting to localhost:9092
peers {
  id: "12D3KooWRkZMjGNrQfRyeKQC9U58cUwAfyQMtjNsupixkBFag8AY"
  connections {
    address: "/ip4/34.150.215.241/tcp/5000"
    uptime: "29.348584085s"
    outbound: true
  }
  tags: "bootnode"
}
Rpc succeeded with OK status
```

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@fasmat fasmat self-assigned this Aug 21, 2023
@codecov
Copy link

codecov bot commented Aug 21, 2023 •
edited

Codecov Report

Merging #4884 (bf8082c) into develop (b23bc52) will decrease coverage by 0.1%.
Report is 1 commits behind head on develop.
The diff coverage is 0.0%.

❗ Current head bf8082c differs from pull request most recent head ca76f04. Consider uploading reports for the commit ca76f04 to get more accurate results

@@            Coverage Diff            @@
##           develop   #4884     +/-   ##
=========================================
- Coverage     76.8%   76.8%   -0.1%     
=========================================
  Files          261     261             
  Lines        30160   30235     +75     
=========================================
+ Hits         23189   23224     +35     
- Misses        5476    5508     +32     
- Partials      1495    1503      +8
Files Changed Coverage Δ
api/grpcserver/smesher_service.go 67.1% <0.0%> (-0.4%) ⬇️

... and 10 files with indirect coverage changes

poszu
poszu approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@poszu poszu left a comment

Choose a reason for hiding this comment

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

How about a unit test to cover it?

@fasmat
Copy link
Member Author

fasmat commented Aug 21, 2023 •
edited

How about a unit test to cover it?

already included in the change 🙂

@fasmat fasmat changed the base branch from develop to v1.1 August 21, 2023 12:36
@fasmat fasmat merged commit 5a0c538 into v1.1 Aug 21, 2023
10 of 14 checks passed
@fasmat fasmat deleted the fix-nil-pointer-grpc branch August 21, 2023 12:37
@fasmat fasmat restored the fix-nil-pointer-grpc branch August 21, 2023 12:37
@fasmat fasmat mentioned this pull request Aug 21, 2023
Merged
@bors bors bot deleted the fix-nil-pointer-grpc branch August 21, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poszu poszu poszu approved these changes

@countvonzero countvonzero Awaiting requested review from countvonzero

@dshulyak dshulyak Awaiting requested review from dshulyak dshulyak is a code owner

Assignees

@fasmat fasmat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fasmat @poszu @piersy