Skip to content

Commit

Permalink
chore: fix panics picked up by Codeql
Browse files Browse the repository at this point in the history
# Related Github tickets

- VolumeFi#1591

# Background

Clanup possible runtime panics picked up by CodeQL analysis.

# Testing completed

- [ ] test coverage exists or has been added/updated
- [ ] tested in a private testnet

# Breaking changes

- [ ] I have checked my code for breaking changes
- [ ] If there are breaking changes, there is a supporting migration.
  • Loading branch information
maharifu authored May 30, 2024
1 parent a68fc9d commit c073ecc
Show file tree
Hide file tree
Showing 9 changed files with 114 additions and 33 deletions.
4 changes: 4 additions & 0 deletions util/keeper/errors.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package keeper

import (
"errors"

"github.com/VolumeFi/whoops"
)

const (
ErrNotFound = whoops.Errorf("item (%T) not found in store: %s")
)

var ErrUnknownPanic = errors.New("unknown panic")
4 changes: 2 additions & 2 deletions x/consensus/keeper/concensus_keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ func (k Keeper) getConsensusQueue(ctx context.Context, queueTypeName string) (co
}

if opts.Batched {
return consensus.NewBatchQueue(opts.QueueOptions), nil
return consensus.NewBatchQueue(opts.QueueOptions)
}
return consensus.NewQueue(opts.QueueOptions), nil
return consensus.NewQueue(opts.QueueOptions)
}
return nil, ErrConsensusQueueNotImplemented.Format(queueTypeName)
}
Expand Down
37 changes: 30 additions & 7 deletions x/consensus/keeper/consensus/batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,21 @@ type BatchQueue struct {
batchedTypeChecker types.TypeChecker
}

func NewBatchQueue(qo QueueOptions) BatchQueue {
func NewBatchQueue(qo QueueOptions) (BatchQueue, error) {
staticTypeCheck := qo.TypeCheck
batchedTypeCheck := types.BatchedTypeChecker(staticTypeCheck)

qo.TypeCheck = batchedTypeCheck

base, err := NewQueue(qo)
if err != nil {
return BatchQueue{}, err
}

return BatchQueue{
base: NewQueue(qo),
base: base,
batchedTypeChecker: staticTypeCheck,
}
}, nil
}

func (c BatchQueue) Put(ctx context.Context, msg ConsensusMsg, opts *PutOptions) (uint64, error) {
Expand All @@ -50,13 +56,24 @@ func (c BatchQueue) Put(ctx context.Context, msg ConsensusMsg, opts *PutOptions)
if err != nil {
return 0, err
}
c.batchQueue(sdkCtx).Set(sdk.Uint64ToBigEndian(newID), data)

batchQueue, err := c.batchQueue(sdkCtx)
if err != nil {
return 0, err
}

batchQueue.Set(sdk.Uint64ToBigEndian(newID), data)
return newID, nil
}

func (c BatchQueue) ProcessBatches(ctx context.Context) error {
sdkCtx := sdk.UnwrapSDKContext(ctx)
queue := c.batchQueue(sdkCtx)

queue, err := c.batchQueue(sdkCtx)
if err != nil {
return err
}

deleteKeys := [][]byte{}

iterator := queue.Iterator(nil, nil)
Expand Down Expand Up @@ -104,10 +121,16 @@ func (c BatchQueue) ProcessBatches(ctx context.Context) error {
}

// batchQueue returns queue of messages that have been batched
func (c BatchQueue) batchQueue(ctx context.Context) prefix.Store {
func (c BatchQueue) batchQueue(ctx context.Context) (prefix.Store, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
store := c.base.qo.Sg.Store(sdkCtx)
return prefix.NewStore(store, []byte("batching:"+c.base.signingQueueKey()))

key, err := c.base.signingQueueKey()
if err != nil {
return prefix.Store{}, err
}

return prefix.NewStore(store, []byte("batching:"+key)), nil
}

func (c BatchQueue) AddSignature(ctx context.Context, id uint64, signData *types.SignData) error {
Expand Down
2 changes: 1 addition & 1 deletion x/consensus/keeper/consensus/batch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func TestBatching(t *testing.T) {

sg := keeperutil.SimpleStoreGetter(stateStore.GetKVStore(storeKey))
msgType := &types.SimpleMessage{}
cq := NewBatchQueue(
cq, _ := NewBatchQueue(
QueueOptions{
QueueTypeName: "simple-message",
Sg: sg,
Expand Down
81 changes: 61 additions & 20 deletions x/consensus/keeper/consensus/consensus.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package consensus
import (
"bytes"
"context"
"errors"
"fmt"

"cosmossdk.io/math"
Expand All @@ -13,6 +14,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/gogoproto/proto"
keeperutil "github.com/palomachain/paloma/util/keeper"
"github.com/palomachain/paloma/util/liblog"
"github.com/palomachain/paloma/x/consensus/types"
)

Expand Down Expand Up @@ -94,31 +96,34 @@ func ApplyOpts(opts *QueueOptions, fncs ...OptFnc) *QueueOptions {
return opts
}

func NewQueue(qo QueueOptions) Queue {
func NewQueue(qo QueueOptions) (Queue, error) {
if qo.TypeCheck == nil {
panic("TypeCheck can't be nil")
return Queue{}, ErrNilTypeCheck
}

if qo.BytesToSignCalculator == nil {
panic("BytesToSignCalculator can't be nil")
return Queue{}, ErrNilBytesToSignCalculator
}

if qo.VerifySignature == nil {
panic("VerifySignature can't be nil")
return Queue{}, ErrNilVerifySignature
}

if len(qo.ChainType) == 0 {
panic("chain type can't be empty")
return Queue{}, ErrEmptyChainType
}

if len(qo.ChainReferenceID) == 0 {
panic("chain id can't be empty")
return Queue{}, ErrEmptyChainReferenceID
}

if len(qo.QueueTypeName) == 0 {
panic("queue type name can't be empty")
return Queue{}, ErrEmptyQueueTypeName
}

return Queue{
qo: qo,
}
}, nil
}

// Put puts raw message into a signing queue.
Expand Down Expand Up @@ -167,7 +172,12 @@ func (c Queue) Put(ctx context.Context, msg ConsensusMsg, opts *PutOptions) (uin
func (c Queue) GetAll(ctx context.Context) ([]types.QueuedSignedMessageI, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
var msgs []types.QueuedSignedMessageI
queue := c.queue(sdkCtx)

queue, err := c.queue(sdkCtx)
if err != nil {
return nil, err
}

iterator := queue.Iterator(nil, nil)
defer iterator.Close()

Expand Down Expand Up @@ -320,7 +330,12 @@ func (c Queue) Remove(ctx context.Context, msgID uint64) error {
if err != nil {
return err
}
queue := c.queue(sdkCtx)

queue, err := c.queue(sdkCtx)
if err != nil {
return err
}

queue.Delete(sdk.Uint64ToBigEndian(msgID))

keeperutil.EmitEvent(keeperutil.ModuleNameFunc(types.ModuleName), sdkCtx, types.ItemRemovedEventKey,
Expand All @@ -333,7 +348,12 @@ func (c Queue) Remove(ctx context.Context, msgID uint64) error {
// getMsgByID given a message ID, it returns the message
func (c Queue) GetMsgByID(ctx context.Context, id uint64) (types.QueuedSignedMessageI, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
queue := c.queue(sdkCtx)

queue, err := c.queue(sdkCtx)
if err != nil {
return nil, err
}

data := queue.Get(sdk.Uint64ToBigEndian(id))

if data == nil {
Expand All @@ -357,23 +377,36 @@ func (c Queue) save(ctx context.Context, msg types.QueuedSignedMessageI) error {
if err != nil {
return err
}
c.queue(ctx).Set(sdk.Uint64ToBigEndian(msg.GetId()), data)

q, err := c.queue(ctx)
if err != nil {
return err
}

q.Set(sdk.Uint64ToBigEndian(msg.GetId()), data)
return nil
}

// queue is a simple helper function to return the queue store
func (c Queue) queue(ctx context.Context) prefix.Store {
func (c Queue) queue(ctx context.Context) (prefix.Store, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
store := c.qo.Sg.Store(sdkCtx)
return prefix.NewStore(store, []byte(c.signingQueueKey()))

key, err := c.signingQueueKey()
if err != nil {
return prefix.Store{}, err
}

return prefix.NewStore(store, []byte(key)), nil
}

// signingQueueKey builds a key for the store where are we going to store those.
func (c Queue) signingQueueKey() string {
func (c Queue) signingQueueKey() (string, error) {
if c.qo.QueueTypeName == "" {
panic("queueTypeName can't be empty")
return "", ErrEmptyQueueTypeName
}
return fmt.Sprintf("%s-%s", consensusQueueSigningKey, c.qo.QueueTypeName)

return fmt.Sprintf("%s-%s", consensusQueueSigningKey, c.qo.QueueTypeName), nil
}

func (c Queue) ChainInfo() (types.ChainType, string) {
Expand All @@ -383,14 +416,22 @@ func (c Queue) ChainInfo() (types.ChainType, string) {
func RemoveQueueCompletely(ctx context.Context, cq Queuer) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
var store storetypes.KVStore
var err error

switch typ := cq.(type) {
case Queue:
store = typ.queue(sdkCtx)
store, err = typ.queue(sdkCtx)
case BatchQueue:
store = typ.batchQueue(sdkCtx)
store, err = typ.batchQueue(sdkCtx)
default:
panic("cq type is unknown!")
err = errors.New("cq type is unknown!")
}

if err != nil {
liblog.FromSDKLogger(sdkCtx.Logger()).WithError(err).Error("Error while removing the queue")
return
}

iterator := store.Iterator(nil, nil)
deleteKeys := [][]byte{}
for ; iterator.Valid(); iterator.Valid() {
Expand Down
11 changes: 11 additions & 0 deletions x/consensus/keeper/consensus/errors.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package consensus

import (
"errors"

"github.com/VolumeFi/whoops"
)

Expand All @@ -14,3 +16,12 @@ const (

ErrAttestorNotSetForMessage = whoops.Errorf("attestator must be set for message: %T")
)

var (
ErrNilTypeCheck = errors.New("TypeCheck can't be nil")
ErrNilBytesToSignCalculator = errors.New("BytesToSignCalculator can't be nil")
ErrNilVerifySignature = errors.New("VerifySignature can't be nil")
ErrEmptyChainType = errors.New("chain type can't be empty")
ErrEmptyChainReferenceID = errors.New("chain id can't be empty")
ErrEmptyQueueTypeName = errors.New("queue type name can't be empty")
)
3 changes: 2 additions & 1 deletion x/evm/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,8 @@ func (k Keeper) PublishSnapshotToAllChains(ctx context.Context, snapshot *valset
func (k Keeper) OnSnapshotBuilt(ctx context.Context, snapshot *valsettypes.Snapshot) {
err := k.PublishSnapshotToAllChains(ctx, snapshot, false)
if err != nil {
panic(err)
k.Logger(ctx).WithError(err).Warn("Failed to publish snapshot to all chains.")
return
}

k.TryDeployingLastCompassContractToAllChains(ctx)
Expand Down
3 changes: 2 additions & 1 deletion x/evm/keeper/scheduler_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ func (k Keeper) XChainType() xchain.Type {
func (k Keeper) XChainReferenceIDs(ctx context.Context) []xchain.ReferenceID {
chainInfos, err := k.GetAllChainInfos(ctx)
if err != nil {
panic(err)
k.Logger(ctx).WithError(err).Warn("Failed to get all chains infos")
return nil
}

return slice.Map(chainInfos, func(ci *types.ChainInfo) xchain.ReferenceID {
Expand Down
2 changes: 1 addition & 1 deletion x/valset/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ func (k Keeper) Jail(_ctx context.Context, valAddr sdk.ValAddress, reason string
case string:
jailingErr = whoops.String(t)
default:
panic(r)
jailingErr = keeperutil.ErrUnknownPanic
}
}()

Expand Down

0 comments on commit c073ecc

Please sign in to comment.