diff --git a/CHANGELOG.md b/CHANGELOG.md index bfa86fa58d..a3ca16392e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (baseapp) [#24027](https://github.com/cosmos/cosmos-sdk/pull/24027) Ensure that `BaseApp.Init` checks that the commit multistore is set to protect against nil dereferences. * (x/group) [GHSA-47ww-ff84-4jrg](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-47ww-ff84-4jrg) Fix x/group can halt when erroring in EndBlocker * (x/distribution) [#23934](https://github.com/cosmos/cosmos-sdk/pull/23934) Fix vulnerability in `incrementReferenceCount` in distribution. * (baseapp) [#23879](https://github.com/cosmos/cosmos-sdk/pull/23879) Ensure finalize block response is not empty in the defer check of FinalizeBlock to avoid panic by nil pointer. diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index c2cf41989d..45b80904c0 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -421,16 +421,16 @@ func (app *BaseApp) Init() error { panic("cannot call initFromMainStore: baseapp already sealed") } + if app.cms == nil { + return errors.New("commit multi-store must not be nil") + } + emptyHeader := cmtproto.Header{ChainID: app.chainID} // needed for the export command which inits from store but never calls initchain app.setState(execModeCheck, emptyHeader) app.Seal() - if app.cms == nil { - return errors.New("commit multi-store must not be nil") - } - return app.cms.GetPruning().Validate() } diff --git a/baseapp/regression_test.go b/baseapp/regression_test.go new file mode 100644 index 0000000000..c93d732299 --- /dev/null +++ b/baseapp/regression_test.go @@ -0,0 +1,41 @@ +package baseapp + +import ( + "testing" + + dbm "github.com/cosmos/cosmos-db" + "github.com/stretchr/testify/require" + + "cosmossdk.io/log" + "cosmossdk.io/store" + storemetrics "cosmossdk.io/store/metrics" +) + +// Ensures that error checks are performed before sealing the app. +// Please see https://github.com/cosmos/cosmos-sdk/issues/18726 +func TestNilCmsCheckBeforeSeal(t *testing.T) { + app := new(BaseApp) + + // 1. Invoking app.Init with a nil cms MUST not seal the app + // and should return an error firstly, which can later be reversed. + for i := 0; i < 10; i++ { // N times, the app shouldn't be sealed. + err := app.Init() + require.Error(t, err) + require.Contains(t, err.Error(), "commit multi-store must not be nil") + require.False(t, app.IsSealed(), "the app MUST not be sealed") + } + + // 2. Now that we've figured out and gotten back an error, let's rectify the problem. + // and we should be able to set the commit multistore then reinvoke app.Init successfully! + db := dbm.NewMemDB() + logger := log.NewTestLogger(t) + app.cms = store.NewCommitMultiStore(db, logger, storemetrics.NewNoOpMetrics()) + err := app.Init() + require.Nil(t, err, "app.Init MUST now succeed") + require.True(t, app.IsSealed(), "the app must now be sealed") + + // 3. Now we should expect a panic because the app is sealed. + require.Panics(t, func() { + _ = app.Init() + }) +}