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 nil pointer dereference #4884

Merged
merged 2 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 38 additions & 1 deletion api/grpcserver/admin_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/durationpb"

"github.com/spacemeshos/go-spacemesh/checkpoint"
"github.com/spacemeshos/go-spacemesh/common/types"
Expand All @@ -31,14 +32,16 @@
logger log.Logger
db *sql.Database
dataDir string
p peers
}

// NewAdminService creates a new admin grpc service.
func NewAdminService(db *sql.Database, dataDir string, lg log.Logger) *AdminService {
func NewAdminService(db *sql.Database, dataDir string, lg log.Logger, p peers) *AdminService {
return &AdminService{
logger: lg,
db: db,
dataDir: dataDir,
p: p,
}
}

Expand Down Expand Up @@ -128,3 +131,37 @@
}
}
}

func (a AdminService) PeerInfoStream(_ *empty.Empty, stream pb.AdminService_PeerInfoStreamServer) error {
for _, p := range a.p.GetPeers() {
select {
case <-stream.Context().Done():
return nil

Check warning on line 139 in api/grpcserver/admin_service.go

View check run for this annotation

Codecov / codecov/patch

api/grpcserver/admin_service.go#L138-L139

Added lines #L138 - L139 were not covered by tests
default:
info := a.p.ConnectedPeerInfo(p)
// There is no guarantee that the peers originally returned will still
// be connected by the time we call ConnectedPeerInfo.
if info == nil {
continue

Check warning on line 145 in api/grpcserver/admin_service.go

View check run for this annotation

Codecov / codecov/patch

api/grpcserver/admin_service.go#L145

Added line #L145 was not covered by tests
}
connections := make([]*pb.ConnectionInfo, len(info.Connections))
for j, c := range info.Connections {
connections[j] = &pb.ConnectionInfo{
Address: c.Address.String(),
Uptime: durationpb.New(c.Uptime),
Outbound: c.Outbound,
}
}
err := stream.Send(&pb.PeerInfo{
Id: info.ID.String(),
Connections: connections,
Tags: info.Tags,
})
if err != nil {
return fmt.Errorf("send to stream: %w", err)
}

Check warning on line 162 in api/grpcserver/admin_service.go

View check run for this annotation

Codecov / codecov/patch

api/grpcserver/admin_service.go#L161-L162

Added lines #L161 - L162 were not covered by tests
}
}

return nil
}
4 changes: 2 additions & 2 deletions api/grpcserver/admin_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func createMesh(tb testing.TB, db *sql.Database) {
func TestAdminService_Checkpoint(t *testing.T) {
db := sql.InMemory()
createMesh(t, db)
svc := NewAdminService(db, t.TempDir(), logtest.New(t))
svc := NewAdminService(db, t.TempDir(), logtest.New(t), nil)
t.Cleanup(launchServer(t, cfg, svc))

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Expand Down Expand Up @@ -91,7 +91,7 @@ func TestAdminService_Checkpoint(t *testing.T) {

func TestAdminService_CheckpointError(t *testing.T) {
db := sql.InMemory()
svc := NewAdminService(db, t.TempDir(), logtest.New(t))
svc := NewAdminService(db, t.TempDir(), logtest.New(t), nil)
t.Cleanup(launchServer(t, cfg, svc))

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Expand Down
14 changes: 10 additions & 4 deletions api/grpcserver/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ type ServiceAPI interface {

// Server is a very basic grpc server.
type Server struct {
Listener string
logger log.Logger
GrpcServer *grpc.Server
grp errgroup.Group
Listener string
logger log.Logger
// BoundAddress contains the address that the server bound to, useful if
// the server uses a dynamic port. It is set during startup and can be
// safely accessed after Start has completed (I.E. the returned channel has
// been waited on)
BoundAddress string
GrpcServer *grpc.Server
grp errgroup.Group
}

// New creates and returns a new Server with port and interface.
Expand Down Expand Up @@ -51,6 +56,7 @@ func (s *Server) Start() error {
s.logger.Error("error listening: %v", err)
return err
}
s.BoundAddress = lis.Addr().String()
reflection.Register(s.GrpcServer)
s.grp.Go(func() error {
if err := s.GrpcServer.Serve(lis); err != nil {
Expand Down
6 changes: 6 additions & 0 deletions api/grpcserver/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ type peerCounter interface {
PeerCount() uint64
}

// Peers is an api to get peer related info.
type peers interface {
ConnectedPeerInfo(p2p.Peer) *p2p.PeerInfo
GetPeers() []p2p.Peer
}

// genesisTimeAPI is an API to get genesis time and current layer of the system.
type genesisTimeAPI interface {
GenesisTime() time.Time
Expand Down
51 changes: 51 additions & 0 deletions api/grpcserver/mocks.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions api/grpcserver/smesher_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ func statusToPbStatus(status *activation.PostSetupStatus) *pb.PostSetupStatus {
if status.LastOpts != nil {
var providerID *uint32
if status.LastOpts.ProviderID.Value() != nil {
providerID = new(uint32)
*providerID = uint32(*status.LastOpts.ProviderID.Value())
}

Expand Down
82 changes: 82 additions & 0 deletions api/grpcserver/smesher_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,85 @@ func TestSmesherService_PostSetupProviders(t *testing.T) {
require.EqualValues(t, providers[1].ID, resp.Providers[1].Id)
require.Equal(t, uint64(100_000), resp.Providers[1].Performance)
}

func TestSmesherService_PostSetupStatus(t *testing.T) {
t.Run("completed", func(t *testing.T) {
ctrl := gomock.NewController(t)
postSetupProvider := activation.NewMockpostSetupProvider(ctrl)
smeshingProvider := activation.NewMockSmeshingProvider(ctrl)
svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second, activation.DefaultPostSetupOpts(), logtest.New(t).WithName("grpc.Smesher"))

postSetupProvider.EXPECT().Status().Return(&activation.PostSetupStatus{
State: activation.PostSetupStateComplete,
NumLabelsWritten: 1_000,
LastOpts: nil,
})
resp, err := svc.PostSetupStatus(context.Background(), &emptypb.Empty{})
require.NoError(t, err)
require.Equal(t, pb.PostSetupStatus_STATE_COMPLETE, resp.Status.State)
require.EqualValues(t, 1_000, resp.Status.NumLabelsWritten)
require.Nil(t, resp.Status.Opts)
})

t.Run("completed with last Opts", func(t *testing.T) {
ctrl := gomock.NewController(t)
postSetupProvider := activation.NewMockpostSetupProvider(ctrl)
smeshingProvider := activation.NewMockSmeshingProvider(ctrl)
svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second, activation.DefaultPostSetupOpts(), logtest.New(t).WithName("grpc.Smesher"))

id := activation.PostProviderID{}
id.SetInt64(1)
opts := activation.PostSetupOpts{
DataDir: "data-dir",
NumUnits: 4,
MaxFileSize: 1024,
ProviderID: id,
Throttle: true,
}
postSetupProvider.EXPECT().Status().Return(&activation.PostSetupStatus{
State: activation.PostSetupStateComplete,
NumLabelsWritten: 1_000,
LastOpts: &opts,
})
resp, err := svc.PostSetupStatus(context.Background(), &emptypb.Empty{})
require.NoError(t, err)
require.Equal(t, pb.PostSetupStatus_STATE_COMPLETE, resp.Status.State)
require.EqualValues(t, 1_000, resp.Status.NumLabelsWritten)
require.Equal(t, "data-dir", resp.Status.Opts.DataDir)
require.EqualValues(t, 4, resp.Status.Opts.NumUnits)
require.EqualValues(t, 1024, resp.Status.Opts.MaxFileSize)
require.EqualValues(t, 1, *resp.Status.Opts.ProviderId)
require.True(t, resp.Status.Opts.Throttle)
})

t.Run("in progress", func(t *testing.T) {
ctrl := gomock.NewController(t)
postSetupProvider := activation.NewMockpostSetupProvider(ctrl)
smeshingProvider := activation.NewMockSmeshingProvider(ctrl)
svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second, activation.DefaultPostSetupOpts(), logtest.New(t).WithName("grpc.Smesher"))

id := activation.PostProviderID{}
id.SetInt64(100)
opts := activation.PostSetupOpts{
DataDir: "data-dir",
NumUnits: 4,
MaxFileSize: 1024,
ProviderID: id,
Throttle: false,
}
postSetupProvider.EXPECT().Status().Return(&activation.PostSetupStatus{
State: activation.PostSetupStateInProgress,
NumLabelsWritten: 1_000,
LastOpts: &opts,
})
resp, err := svc.PostSetupStatus(context.Background(), &emptypb.Empty{})
require.NoError(t, err)
require.Equal(t, pb.PostSetupStatus_STATE_IN_PROGRESS, resp.Status.State)
require.EqualValues(t, 1_000, resp.Status.NumLabelsWritten)
require.Equal(t, "data-dir", resp.Status.Opts.DataDir)
require.EqualValues(t, 4, resp.Status.Opts.NumUnits)
require.EqualValues(t, 1024, resp.Status.Opts.MaxFileSize)
require.EqualValues(t, 100, *resp.Status.Opts.ProviderId)
require.False(t, resp.Status.Opts.Throttle)
})
}
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ require (
github.com/pyroscope-io/pyroscope v0.37.2
github.com/santhosh-tekuri/jsonschema/v5 v5.3.1
github.com/seehuhn/mt19937 v1.0.0
github.com/spacemeshos/api/release/go v1.19.0
github.com/spacemeshos/api/release/go v1.20.0
github.com/spacemeshos/economics v0.1.0
github.com/spacemeshos/fixed v0.1.0
github.com/spacemeshos/go-scale v1.1.10
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -606,8 +606,8 @@ github.com/smartystreets/goconvey v1.7.2 h1:9RBaZCeXEQ3UselpuwUQHltGVXvdwm6cv1hg
github.com/smartystreets/goconvey v1.7.2/go.mod h1:Vw0tHAZW6lzCRk3xgdin6fKYcG+G3Pg9vgXWeJpQFMM=
github.com/sourcegraph/annotate v0.0.0-20160123013949-f4cad6c6324d/go.mod h1:UdhH50NIW0fCiwBSr0co2m7BnFLdv4fQTgdqdJTHFeE=
github.com/sourcegraph/syntaxhighlight v0.0.0-20170531221838-bd320f5d308e/go.mod h1:HuIsMU8RRBOtsCgI77wP899iHVBQpCmg4ErYMZB+2IA=
github.com/spacemeshos/api/release/go v1.19.0 h1:QPt1nUuSVQ4DfZPNsSAuJpjjYlbS9HJhDunE7K8rn08=
github.com/spacemeshos/api/release/go v1.19.0/go.mod h1:nC5g7IVRNxF8Kl/Tzvr7cQeYBwHN4sMTVsIEa6Ebtl4=
github.com/spacemeshos/api/release/go v1.20.0 h1:9HEWDTXyE5mSfca8C5tIXauI1igeUO8CLm5LM6X6y0k=
github.com/spacemeshos/api/release/go v1.20.0/go.mod h1:nC5g7IVRNxF8Kl/Tzvr7cQeYBwHN4sMTVsIEa6Ebtl4=
github.com/spacemeshos/economics v0.1.0 h1:PJAKbhBKqbbdCYTB29pkmc8sYqK3pKUAiuAvQxuSJEg=
github.com/spacemeshos/economics v0.1.0/go.mod h1:Bz0wRDwCOUP1A6w3cPW6iuUBGME8Tz48sIriYiohsBg=
github.com/spacemeshos/fixed v0.1.0 h1:20KIGvxLlAsuidQrvuwwHe6PrvqeTKzbJIsScbmnUPQ=
Expand Down
79 changes: 79 additions & 0 deletions node/adminservice_api_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package node

import (
"context"
"io"
"testing"
"time"

"github.com/golang/protobuf/ptypes/empty"
"github.com/libp2p/go-libp2p/core/peer"
pb "github.com/spacemeshos/api/release/go/spacemesh/v1"
"github.com/stretchr/testify/require"

"github.com/spacemeshos/go-spacemesh/api/grpcserver"
"github.com/spacemeshos/go-spacemesh/config"
"github.com/spacemeshos/go-spacemesh/log/logtest"
)

func TestPeerInfoApi(t *testing.T) {
cfg := config.DefaultTestConfig()
cfg.P2P.DisableNatPort = true
cfg.P2P.Listen = "/ip4/127.0.0.1/tcp/0"

cfg.API.PublicListener = "0.0.0.0:0"
cfg.API.PrivateServices = nil
cfg.API.PublicServices = []string{grpcserver.Admin}
l := logtest.New(t)
networkSize := 3
network := NewTestNetwork(t, cfg, l, networkSize)
infos := make([][]*pb.PeerInfo, networkSize)
for i, app := range network {
adminapi := pb.NewAdminServiceClient(app.Conn)
ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
defer cancel()

streamClient, err := adminapi.PeerInfoStream(ctx, &empty.Empty{})
require.NoError(t, err)
for {
info, err := streamClient.Recv()
if err == io.EOF {
break
}
require.NoError(t, err)
infos[i] = append(infos[i], info)
}
}

for i, app := range network {
for j, innerApp := range network {
if j == i {
continue
}
peers := infos[i]
require.Len(t, peers, networkSize-1, "expecting each node to have connections to all other nodes")
peer := getPeerInfo(peers, innerApp.host.ID())
require.NotNil(t, peer, "info is missing connection to %v")
require.Len(t, peer.Connections, 1, "expecting only 1 connection to each peer")
require.Equal(t, innerApp.host.Addrs()[0].String(), peer.Connections[0].Address, "connection address should match address of peer")
require.Greater(t, peer.Connections[0].Uptime.AsDuration(), time.Duration(0), "uptime should be set")
outbound := peer.Connections[0].Outbound

// Check that outbound matches with the other side of the connection
otherSide := getPeerInfo(infos[j], app.host.ID())
require.NotNil(t, peer, "one side missing peer connection")
require.Len(t, otherSide.Connections, 1, "expecting only 1 connection to each peer")
require.Equal(t, outbound, !otherSide.Connections[0].Outbound, "expecting pairwise connections to agree on outbound direction")
}
}
}

func getPeerInfo(peers []*pb.PeerInfo, id peer.ID) *pb.PeerInfo {
str := id.String()
for _, p := range peers {
if str == p.Id {
return p
}
}
return nil
}