From ec61581390a687918e17d0bf96cc44040389c994 Mon Sep 17 00:00:00 2001 From: George Date: Thu, 21 Apr 2022 15:23:31 -0700 Subject: [PATCH] services/horizon: Pass through nil ExtraSigners to avoid nil pointer deref (#4349) --- .../internal/db2/history/fee_bump_scenario.go | 3 +- .../transaction_batch_insert_builder.go | 4 + .../internal/db2/history/transaction_test.go | 27 ++++--- .../transaction_preconditions_test.go | 78 ++++++++++++++++++- .../internal/resourceadapter/transaction.go | 24 +++--- 5 files changed, 108 insertions(+), 28 deletions(-) diff --git a/services/horizon/internal/db2/history/fee_bump_scenario.go b/services/horizon/internal/db2/history/fee_bump_scenario.go index 65d1e7009d..b555aa9355 100644 --- a/services/horizon/internal/db2/history/fee_bump_scenario.go +++ b/services/horizon/internal/db2/history/fee_bump_scenario.go @@ -9,7 +9,6 @@ import ( sq "github.com/Masterminds/squirrel" "github.com/guregu/null" - "github.com/lib/pq" "github.com/stellar/go/ingest" "github.com/stellar/go/network" @@ -314,7 +313,7 @@ func FeeBumpScenario(tt *test.T, q *Q, successful bool) FeeBumpFixture { Memo: null.NewString("", false), TimeBounds: TimeBounds{Lower: null.IntFrom(2), Upper: null.IntFrom(4)}, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Signatures: signatures(fixture.Envelope.FeeBumpSignatures()), InnerSignatures: signatures(fixture.Envelope.Signatures()), Successful: successful, diff --git a/services/horizon/internal/db2/history/transaction_batch_insert_builder.go b/services/horizon/internal/db2/history/transaction_batch_insert_builder.go index e22fdbf096..1adb2aaa34 100644 --- a/services/horizon/internal/db2/history/transaction_batch_insert_builder.go +++ b/services/horizon/internal/db2/history/transaction_batch_insert_builder.go @@ -249,6 +249,10 @@ func formatUint32(u *xdr.Uint32) null.Int { } func formatSigners(s []xdr.SignerKey) pq.StringArray { + if s == nil { + return nil + } + signers := make([]string, len(s)) for i, key := range s { signers[i] = key.Address() diff --git a/services/horizon/internal/db2/history/transaction_test.go b/services/horizon/internal/db2/history/transaction_test.go index 7c491fe54d..5f65451155 100644 --- a/services/horizon/internal/db2/history/transaction_test.go +++ b/services/horizon/internal/db2/history/transaction_test.go @@ -7,7 +7,6 @@ import ( sq "github.com/Masterminds/squirrel" "github.com/guregu/null" - "github.com/lib/pq" "github.com/stellar/go/xdr" "github.com/stellar/go/ingest" @@ -364,7 +363,7 @@ func TestInsertTransaction(t *testing.T) { Successful: success, TimeBounds: nullTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, }, }, }, @@ -400,7 +399,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("", false), TimeBounds: nullTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -437,7 +436,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("", false), TimeBounds: nullTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: false, }, }, @@ -475,7 +474,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("test memo", true), TimeBounds: infiniteTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -512,7 +511,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("test memo", true), TimeBounds: infiniteTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -549,7 +548,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("123", true), TimeBounds: nullTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -586,7 +585,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("fi3vINWiGla+KkV7ZI9wLuGviJ099leQ6SoFCB6fq/E=", true), TimeBounds: infiniteTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -623,7 +622,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("zdjArlILa/LNv7o7lo/qv5+fVVPNl0yPgZQWB6u+gL4=", true), TimeBounds: infiniteTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -661,7 +660,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("", false), TimeBounds: timeBoundWithMin, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -698,7 +697,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("", false), TimeBounds: timeBoundWithMax, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -735,7 +734,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("", false), TimeBounds: timeboundsWithMinAndMax, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -778,7 +777,7 @@ func TestInsertTransaction(t *testing.T) { MinAccountSequence: null.Int{}, MinAccountSequenceAge: null.StringFrom("10"), MinAccountSequenceLedgerGap: null.IntFrom(2), - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, @@ -815,7 +814,7 @@ func TestInsertTransaction(t *testing.T) { Memo: null.NewString("", false), TimeBounds: nullTimeBounds, LedgerBounds: LedgerBounds{Null: true}, - ExtraSigners: pq.StringArray{}, + ExtraSigners: nil, Successful: success, }, }, diff --git a/services/horizon/internal/integration/transaction_preconditions_test.go b/services/horizon/internal/integration/transaction_preconditions_test.go index 7d47a347b6..ea50e15f42 100644 --- a/services/horizon/internal/integration/transaction_preconditions_test.go +++ b/services/horizon/internal/integration/transaction_preconditions_test.go @@ -1,6 +1,8 @@ package integration import ( + "bytes" + "encoding/base64" "fmt" "strconv" "sync" @@ -9,9 +11,11 @@ import ( sdk "github.com/stellar/go/clients/horizonclient" "github.com/stellar/go/keypair" + "github.com/stellar/go/network" "github.com/stellar/go/protocols/horizon" "github.com/stellar/go/services/horizon/internal/test/integration" "github.com/stellar/go/txnbuild" + "github.com/stellar/go/xdr" "github.com/stretchr/testify/assert" ) @@ -286,7 +290,6 @@ func TestTransactionPreconditionsMinSequenceNumberLedgerGap(t *testing.T) { } func buildTXParams(master *keypair.Full, masterAccount txnbuild.Account, txSequence int64) txnbuild.TransactionParams { - return txnbuild.TransactionParams{ SourceAccount: &txnbuild.SimpleAccount{ AccountID: masterAccount.GetAccountID(), @@ -334,3 +337,76 @@ func TestTransactionPreconditionsAccountV3Fields(t *testing.T) { tt.Equal(uint32(tx.Ledger), account.SequenceLedger) tt.Equal(strconv.FormatInt(tx.LedgerCloseTime.Unix(), 10), account.SequenceTime) } + +// TestTransactionWithoutPreconditions ensures that Horizon doesn't break when +// we have a PRECOND_NONE type transaction (which is not possible to submit +// through SDKs, but is absolutely still possible). +func TestTransactionWithoutPreconditions(t *testing.T) { + tt := assert.New(t) + itest := integration.NewTest(t, integration.Config{}) + if itest.GetEffectiveProtocolVersion() < 19 { + t.Skip("Can't run with protocol < 19") + } + + master := itest.Master() + masterAccount := itest.MasterAccount() + seqNum, err := masterAccount.GetSequenceNumber() + tt.NoError(err) + + account := xdr.MuxedAccount{} + tt.NoError(account.SetEd25519Address(master.Address())) + + payment := txnbuild.Payment{ // dummy op + Destination: master.Address(), + Amount: "1000", + Asset: txnbuild.NativeAsset{}, + } + paymentOp, err := payment.BuildXDR() + tt.NoError(err) + + envelope := xdr.TransactionEnvelope{ + Type: xdr.EnvelopeTypeEnvelopeTypeTx, + V1: &xdr.TransactionV1Envelope{ + Tx: xdr.Transaction{ + SourceAccount: account, + Fee: xdr.Uint32(1000), + SeqNum: xdr.SequenceNumber(seqNum + 1), + Operations: []xdr.Operation{paymentOp}, + Cond: xdr.Preconditions{ + Type: xdr.PreconditionTypePrecondNone, + }, + }, + Signatures: nil, + }, + } + + // Taken from txnbuild.concatSignatures + h, err := network.HashTransactionInEnvelope(envelope, + integration.StandaloneNetworkPassphrase) + tt.NoError(err) + + sig, err := master.SignDecorated(h[:]) + tt.NoError(err) + + // taken from txnbuild.marshallBinary + var txBytes bytes.Buffer + envelope.V1.Signatures = []xdr.DecoratedSignature{sig} + _, err = xdr.Marshal(&txBytes, envelope) + tt.NoError(err) + b64 := base64.StdEncoding.EncodeToString(txBytes.Bytes()) + + txResp, err := itest.Client().SubmitTransactionXDR(b64) + tt.NoError(err) + + fmt.Println( + "envelopeXDR", txResp.EnvelopeXdr, + "resultXDR", txResp.ResultXdr, + // "feeChangesXDR", txResp.feeChangesXDR, + "metaXDR", txResp.FeeMetaXdr, + "hash", txResp.Hash, + ) + + txResp2, err := itest.Client().TransactionDetail(txResp.Hash) + tt.NoError(err) + tt.Nil(txResp2.Preconditions) +} diff --git a/services/horizon/internal/resourceadapter/transaction.go b/services/horizon/internal/resourceadapter/transaction.go index f27a819696..76922fec6a 100644 --- a/services/horizon/internal/resourceadapter/transaction.go +++ b/services/horizon/internal/resourceadapter/transaction.go @@ -55,26 +55,23 @@ func PopulateTransaction( } dest.Signatures = row.Signatures - if row.HasPreconditions() { - dest.Preconditions = &protocol.TransactionPreconditions{} - } + // If we never use this, we'll remove it later. This just defends us against + // nil dereferences. + dest.Preconditions = &protocol.TransactionPreconditions{} if !row.TimeBounds.Null { // Action needed in release: horizon-v3.0.0: remove ValidBefore and ValidAfter dest.ValidBefore = timeString(row.TimeBounds.Upper) dest.ValidAfter = timeString(row.TimeBounds.Lower) - if dest.Preconditions.TimeBounds == nil { - dest.Preconditions.TimeBounds = &protocol.TransactionPreconditionsTimebounds{} + dest.Preconditions.TimeBounds = &protocol.TransactionPreconditionsTimebounds{ + MaxTime: timeString(row.TimeBounds.Upper), + MinTime: timeString(row.TimeBounds.Lower), } - dest.Preconditions.TimeBounds.MaxTime = timeString(row.TimeBounds.Upper) - dest.Preconditions.TimeBounds.MinTime = timeString(row.TimeBounds.Lower) } if !row.LedgerBounds.Null { - if dest.Preconditions.LedgerBounds == nil { - dest.Preconditions.LedgerBounds = &protocol.TransactionPreconditionsLedgerbounds{} - } + dest.Preconditions.LedgerBounds = &protocol.TransactionPreconditionsLedgerbounds{} if row.LedgerBounds.MinLedger.Valid { dest.Preconditions.LedgerBounds.MinLedger = uint32(row.LedgerBounds.MinLedger.Int64) } @@ -95,7 +92,7 @@ func PopulateTransaction( dest.Preconditions.MinAccountSequenceLedgerGap = uint32(row.MinAccountSequenceLedgerGap.Int64) } - if row.ExtraSigners != nil { + if len(row.ExtraSigners) > 0 { dest.Preconditions.ExtraSigners = row.ExtraSigners } @@ -136,6 +133,11 @@ func PopulateTransaction( dest.Links.Succeeds = lb.Linkf("/transactions?order=desc&cursor=%s", dest.PT) dest.Links.Precedes = lb.Linkf("/transactions?order=asc&cursor=%s", dest.PT) + // If we didn't need the structure, drop it. + if !row.HasPreconditions() { + dest.Preconditions = nil + } + return nil }