Merge pull request #8475 from filecoin-project/fix/deal-idx-oob

fix:sealing:check index out of bounds against correct param length not return length

chain/actors/builtin/market/actor.go.template

@@ -108,7 +108,7 @@ type PublishStorageDealsParams = market0.PublishStorageDealsParams
 type PublishStorageDealsReturn interface {
     DealIDs() ([]abi.DealID, error)
 	// Note that this index is based on the batch of deals that were published, NOT the DealID
-	IsDealValid(index uint64) (bool, error)
+	IsDealValid(index uint64) (bool, int, error)
 }
 
 func DecodePublishStorageDealsReturn(b []byte, nv network.Version) (PublishStorageDealsReturn, error) {

chain/actors/builtin/market/market.go

@@ -197,7 +197,7 @@ type PublishStorageDealsParams = market0.PublishStorageDealsParams
 type PublishStorageDealsReturn interface {
 	DealIDs() ([]abi.DealID, error)
 	// Note that this index is based on the batch of deals that were published, NOT the DealID
-	IsDealValid(index uint64) (bool, error)
+	IsDealValid(index uint64) (bool, int, error)
 }
 
 func DecodePublishStorageDealsReturn(b []byte, nv network.Version) (PublishStorageDealsReturn, error) {

chain/actors/builtin/market/state.go.template

@@ -8,6 +8,11 @@ import (
 	"github.com/ipfs/go-cid"
 	cbg "github.com/whyrusleeping/cbor-gen"
 	"golang.org/x/xerrors"
+	{{if (ge .v 6)}}
+	rlepluslazy "github.com/filecoin-project/go-bitfield/rle"
+	"github.com/filecoin-project/go-bitfield"
+	{{end}}
+
 
 	"github.com/filecoin-project/lotus/chain/actors/adt"
 	"github.com/filecoin-project/lotus/chain/types"
@@ -253,12 +258,29 @@ type publishStorageDealsReturn{{.v}} struct {
 	market{{.v}}.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn{{.v}}) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn{{.v}}) IsDealValid(index uint64) (bool, int, error) {
 	{{if (ge .v 6)}}
-	    return r.ValidDeals.IsSet(index)
+	set, err := r.ValidDeals.IsSet(index)
+	if err != nil || !set {
+		return false, -1, err
+	}
+	maskBf, err := bitfield.NewFromIter(&rlepluslazy.RunSliceIterator{
+		Runs: []rlepluslazy.Run{rlepluslazy.Run{Val: true, Len: index}}})
+	if err != nil {
+		return false, -1, err
+	}
+	before, err := bitfield.IntersectBitField(maskBf, r.ValidDeals)
+	if err != nil {
+		return false, -1, err
+	}
+	outIdx, err := before.Count()
+	if err != nil {
+		return false, -1, err
+	}
+	return set, int(outIdx), nil
 	{{else}}
 	    // PublishStorageDeals only succeeded if all deals were valid in this version of actors
-	    return true, nil
+	    return true, int(index), nil
 	{{end}}
 }
 

chain/actors/builtin/market/v0.go

@@ -246,10 +246,10 @@ type publishStorageDealsReturn0 struct {
 	market0.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn0) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn0) IsDealValid(index uint64) (bool, int, error) {
 
 	// PublishStorageDeals only succeeded if all deals were valid in this version of actors
-	return true, nil
+	return true, int(index), nil
 
 }
 

chain/actors/builtin/market/v2.go

@@ -246,10 +246,10 @@ type publishStorageDealsReturn2 struct {
 	market2.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn2) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn2) IsDealValid(index uint64) (bool, int, error) {
 
 	// PublishStorageDeals only succeeded if all deals were valid in this version of actors
-	return true, nil
+	return true, int(index), nil
 
 }
 

chain/actors/builtin/market/v3.go

@@ -241,10 +241,10 @@ type publishStorageDealsReturn3 struct {
 	market3.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn3) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn3) IsDealValid(index uint64) (bool, int, error) {
 
 	// PublishStorageDeals only succeeded if all deals were valid in this version of actors
-	return true, nil
+	return true, int(index), nil
 
 }
 

chain/actors/builtin/market/v4.go

@@ -241,10 +241,10 @@ type publishStorageDealsReturn4 struct {
 	market4.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn4) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn4) IsDealValid(index uint64) (bool, int, error) {
 
 	// PublishStorageDeals only succeeded if all deals were valid in this version of actors
-	return true, nil
+	return true, int(index), nil
 
 }
 

chain/actors/builtin/market/v5.go

@@ -241,10 +241,10 @@ type publishStorageDealsReturn5 struct {
 	market5.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn5) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn5) IsDealValid(index uint64) (bool, int, error) {
 
 	// PublishStorageDeals only succeeded if all deals were valid in this version of actors
-	return true, nil
+	return true, int(index), nil
 
 }
 

chain/actors/builtin/market/v6.go

@@ -9,6 +9,9 @@ import (
 	cbg "github.com/whyrusleeping/cbor-gen"
 	"golang.org/x/xerrors"
 
+	"github.com/filecoin-project/go-bitfield"
+	rlepluslazy "github.com/filecoin-project/go-bitfield/rle"
+
 	"github.com/filecoin-project/lotus/chain/actors/adt"
 	"github.com/filecoin-project/lotus/chain/types"
 
@@ -241,9 +244,26 @@ type publishStorageDealsReturn6 struct {
 	market6.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn6) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn6) IsDealValid(index uint64) (bool, int, error) {
 
-	return r.ValidDeals.IsSet(index)
+	set, err := r.ValidDeals.IsSet(index)
+	if err != nil || !set {
+		return false, -1, err
+	}
+	maskBf, err := bitfield.NewFromIter(&rlepluslazy.RunSliceIterator{
+		Runs: []rlepluslazy.Run{rlepluslazy.Run{Val: true, Len: index}}})
+	if err != nil {
+		return false, -1, err
+	}
+	before, err := bitfield.IntersectBitField(maskBf, r.ValidDeals)
+	if err != nil {
+		return false, -1, err
+	}
+	outIdx, err := before.Count()
+	if err != nil {
+		return false, -1, err
+	}
+	return set, int(outIdx), nil
 
 }
 

chain/actors/builtin/market/v7.go

@@ -9,6 +9,9 @@ import (
 	cbg "github.com/whyrusleeping/cbor-gen"
 	"golang.org/x/xerrors"
 
+	"github.com/filecoin-project/go-bitfield"
+	rlepluslazy "github.com/filecoin-project/go-bitfield/rle"
+
 	"github.com/filecoin-project/lotus/chain/actors/adt"
 	"github.com/filecoin-project/lotus/chain/types"
 
@@ -241,9 +244,26 @@ type publishStorageDealsReturn7 struct {
 	market7.PublishStorageDealsReturn
 }
 
-func (r *publishStorageDealsReturn7) IsDealValid(index uint64) (bool, error) {
+func (r *publishStorageDealsReturn7) IsDealValid(index uint64) (bool, int, error) {
 
-	return r.ValidDeals.IsSet(index)
+	set, err := r.ValidDeals.IsSet(index)
+	if err != nil || !set {
+		return false, -1, err
+	}
+	maskBf, err := bitfield.NewFromIter(&rlepluslazy.RunSliceIterator{
+		Runs: []rlepluslazy.Run{rlepluslazy.Run{Val: true, Len: index}}})
+	if err != nil {
+		return false, -1, err
+	}
+	before, err := bitfield.IntersectBitField(maskBf, r.ValidDeals)
+	if err != nil {
+		return false, -1, err
+	}
+	outIdx, err := before.Count()
+	if err != nil {
+		return false, -1, err
+	}
+	return set, int(outIdx), nil
 
 }
 

extern/storage-sealing/currentdealinfo.go

@@ -3,6 +3,7 @@ package sealing
 import (
 	"bytes"
 	"context"
+	"fmt"
 
 	"github.com/filecoin-project/go-state-types/network"
 
@@ -138,18 +139,19 @@ func (mgr *CurrentDealInfoManager) dealIDFromPublishDealsMsg(ctx context.Context
 			break
 		}
 	}
+	fmt.Printf("found dealIdx %d\n", dealIdx)
 
 	if dealIdx == -1 {
 		return dealID, nil, xerrors.Errorf("could not find deal in publish deals message %s", publishCid)
 	}
 
-	if dealIdx >= len(dealIDs) {
+	if dealIdx >= len(pubDealsParams.Deals) {
 		return dealID, nil, xerrors.Errorf(
-			"deal index %d out of bounds of deals (len %d) in publish deals message %s",
+			"deal index %d out of bounds of deal proposals (len %d) in publish deals message %s",
 			dealIdx, len(dealIDs), publishCid)
 	}
 
-	valid, err := retval.IsDealValid(uint64(dealIdx))
+	valid, outIdx, err := retval.IsDealValid(uint64(dealIdx))
 	if err != nil {
 		return dealID, nil, xerrors.Errorf("determining deal validity: %w", err)
 	}
@@ -158,7 +160,12 @@ func (mgr *CurrentDealInfoManager) dealIDFromPublishDealsMsg(ctx context.Context
 		return dealID, nil, xerrors.New("deal was invalid at publication")
 	}
 
-	return dealIDs[dealIdx], lookup.TipSetTok, nil
+	// final check against for invalid return value output
+	// should not be reachable from onchain output, only pathological test cases
+	if outIdx >= len(dealIDs) {
+		return dealID, nil, xerrors.Errorf("invalid publish storage deals ret marking %d as valid while only returning %d valid deals in publish deal message %s", outIdx, len(dealIDs), publishCid)
+	}
+	return dealIDs[outIdx], lookup.TipSetTok, nil
 }
 
 func (mgr *CurrentDealInfoManager) CheckDealEquality(ctx context.Context, tok TipSetToken, p1, p2 market.DealProposal) (bool, error) {

extern/storage-sealing/currentdealinfo_test.go

@@ -8,9 +8,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/filecoin-project/go-bitfield"
 	"github.com/filecoin-project/go-state-types/network"
 
 	market0 "github.com/filecoin-project/specs-actors/actors/builtin/market"
+	market7 "github.com/filecoin-project/specs-actors/v7/actors/builtin/market"
 
 	"golang.org/x/net/context"
 	"golang.org/x/xerrors"
@@ -36,6 +38,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 	dummyCid, _ := cid.Parse("bafkqaaa")
 	dummyCid2, _ := cid.Parse("bafkqaab")
 	zeroDealID := abi.DealID(0)
+	anotherDealID := abi.DealID(8)
 	earlierDealID := abi.DealID(9)
 	successDealID := abi.DealID(10)
 	proposal := market.DealProposal{
@@ -58,6 +61,16 @@ func TestGetCurrentDealInfo(t *testing.T) {
 		ClientCollateral:     abi.NewTokenAmount(1),
 		Label:                "other",
 	}
+	anotherProposal := market.DealProposal{
+		PieceCID:             dummyCid2,
+		PieceSize:            abi.PaddedPieceSize(100),
+		Client:               tutils.NewActorAddr(t, "client"),
+		Provider:             tutils.NewActorAddr(t, "provider"),
+		StoragePricePerEpoch: abi.NewTokenAmount(1),
+		ProviderCollateral:   abi.NewTokenAmount(1),
+		ClientCollateral:     abi.NewTokenAmount(1),
+		Label:                "another",
+	}
 	successDeal := &api.MarketDeal{
 		Proposal: proposal,
 		State: market.DealState{
@@ -72,6 +85,13 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			LastUpdatedEpoch: 2,
 		},
 	}
+	anotherDeal := &api.MarketDeal{
+		Proposal: anotherProposal,
+		State: market.DealState{
+			SectorStartEpoch: 1,
+			LastUpdatedEpoch: 2,
+		},
+	}
 
 	type testCaseData struct {
 		searchMessageLookup *MsgLookup
@@ -82,6 +102,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 		expectedDealID      abi.DealID
 		expectedMarketDeal  *api.MarketDeal
 		expectedError       error
+		networkVersion      network.Version
 	}
 	testCases := map[string]testCaseData{
 		"deal lookup succeeds": {
@@ -89,7 +110,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			searchMessageLookup: &MsgLookup{
 				Receipt: MessageReceipt{
 					ExitCode: exitcode.Ok,
-					Return:   makePublishDealsReturnBytes(t, []abi.DealID{successDealID}),
+					Return:   makePublishDealsReturnBytesOldVersion(t, []abi.DealID{successDealID}),
 				},
 			},
 			marketDeals: map[abi.DealID]*api.MarketDeal{
@@ -104,7 +125,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			searchMessageLookup: &MsgLookup{
 				Receipt: MessageReceipt{
 					ExitCode: exitcode.Ok,
-					Return:   makePublishDealsReturnBytes(t, []abi.DealID{earlierDealID, successDealID}),
+					Return:   makePublishDealsReturnBytesOldVersion(t, []abi.DealID{earlierDealID, successDealID}),
 				},
 			},
 			marketDeals: map[abi.DealID]*api.MarketDeal{
@@ -120,7 +141,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			searchMessageLookup: &MsgLookup{
 				Receipt: MessageReceipt{
 					ExitCode: exitcode.Ok,
-					Return:   makePublishDealsReturnBytes(t, []abi.DealID{earlierDealID}),
+					Return:   makePublishDealsReturnBytesOldVersion(t, []abi.DealID{earlierDealID}),
 				},
 			},
 			marketDeals: map[abi.DealID]*api.MarketDeal{
@@ -130,12 +151,12 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			expectedDealID: zeroDealID,
 			expectedError:  xerrors.Errorf("could not find deal in publish deals message %s", dummyCid),
 		},
-		"deal lookup fails mismatch count of deals and return values": {
+		"deal lookup handles invalid actor output with mismatched count of deals and return values": {
 			publishCid: dummyCid,
 			searchMessageLookup: &MsgLookup{
 				Receipt: MessageReceipt{
 					ExitCode: exitcode.Ok,
-					Return:   makePublishDealsReturnBytes(t, []abi.DealID{earlierDealID}),
+					Return:   makePublishDealsReturnBytesOldVersion(t, []abi.DealID{earlierDealID}),
 				},
 			},
 			marketDeals: map[abi.DealID]*api.MarketDeal{
@@ -144,14 +165,52 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			},
 			targetProposal: &proposal,
 			expectedDealID: zeroDealID,
-			expectedError:  xerrors.Errorf("deal index 1 out of bounds of deals (len 1) in publish deals message %s", dummyCid),
+			expectedError:  xerrors.Errorf("invalid publish storage deals ret marking 1 as valid while only returning 1 valid deals in publish deal message %s", dummyCid),
 		},
+
+		"deal lookup fails when deal was not valid and index exceeds output array": {
+			publishCid: dummyCid,
+			searchMessageLookup: &MsgLookup{
+				Receipt: MessageReceipt{
+					ExitCode: exitcode.Ok,
+					Return:   makePublishDealsReturn(t, []abi.DealID{earlierDealID}, []uint64{0}),
+				},
+			},
+			marketDeals: map[abi.DealID]*api.MarketDeal{
+				earlierDealID: earlierDeal,
+				successDealID: successDeal,
+			},
+			targetProposal: &proposal,
+			expectedDealID: zeroDealID,
+			expectedError:  xerrors.Errorf("deal was invalid at publication"),
+			networkVersion: network.Version14,
+		},
+
+		"deal lookup succeeds when theres a separate deal failure": {
+			publishCid: dummyCid,
+			searchMessageLookup: &MsgLookup{
+				Receipt: MessageReceipt{
+					ExitCode: exitcode.Ok,
+					Return:   makePublishDealsReturn(t, []abi.DealID{anotherDealID, successDealID}, []uint64{0, 2}),
+				},
+			},
+			marketDeals: map[abi.DealID]*api.MarketDeal{
+				anotherDealID: anotherDeal,
+				earlierDealID: earlierDeal,
+				successDealID: successDeal,
+			},
+			targetProposal:     &proposal,
+			expectedDealID:     successDealID,
+			expectedMarketDeal: successDeal,
+			networkVersion:     network.Version14,
+		},
+
 		"deal lookup succeeds, target proposal nil, single deal in message": {
 			publishCid: dummyCid,
 			searchMessageLookup: &MsgLookup{
 				Receipt: MessageReceipt{
 					ExitCode: exitcode.Ok,
-					Return:   makePublishDealsReturnBytes(t, []abi.DealID{successDealID}),
+					Return:   makePublishDealsReturnBytesOldVersion(t, []abi.DealID{successDealID}),
 				},
 			},
 			marketDeals: map[abi.DealID]*api.MarketDeal{
@@ -166,7 +225,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 			searchMessageLookup: &MsgLookup{
 				Receipt: MessageReceipt{
 					ExitCode: exitcode.Ok,
-					Return:   makePublishDealsReturnBytes(t, []abi.DealID{earlierDealID, successDealID}),
+					Return:   makePublishDealsReturnBytesOldVersion(t, []abi.DealID{earlierDealID, successDealID}),
 				},
 			},
 			marketDeals: map[abi.DealID]*api.MarketDeal{
@@ -228,6 +287,7 @@ func TestGetCurrentDealInfo(t *testing.T) {
 				SearchMessageLookup: data.searchMessageLookup,
 				SearchMessageErr:    data.searchMessageErr,
 				MarketDeals:         marketDeals,
+				Version:             data.networkVersion,
 			}
 			dealInfoMgr := CurrentDealInfoManager{mockApi}
 
@@ -256,13 +316,21 @@ type CurrentDealInfoMockAPI struct {
 	SearchMessageErr    error
 
 	MarketDeals map[marketDealKey]*api.MarketDeal
+	Version     network.Version
 }
 
 func (mapi *CurrentDealInfoMockAPI) ChainGetMessage(ctx context.Context, c cid.Cid) (*types.Message, error) {
-	var dealIDs []abi.DealID
+	var keys []marketDealKey
+	for k := range mapi.MarketDeals {
+		keys = append(keys, k)
+	}
+	sort.SliceStable(keys, func(i, j int) bool {
+		return keys[i].DealID < keys[j].DealID
+	})
+
 	var deals []market2.ClientDealProposal
-	for k, dl := range mapi.MarketDeals {
+	for _, k := range keys {
-		dealIDs = append(dealIDs, k.DealID)
+		dl := mapi.MarketDeals[k]
 		deals = append(deals, market2.ClientDealProposal{
 			Proposal: market2.DealProposal(dl.Proposal),
 			ClientSignature: crypto.Signature{
@@ -271,15 +339,14 @@ func (mapi *CurrentDealInfoMockAPI) ChainGetMessage(ctx context.Context, c cid.C
 			},
 		})
 	}
-	sort.SliceStable(deals, func(i, j int) bool {
+
-		return dealIDs[i] < dealIDs[j]
-	})
 	buf := new(bytes.Buffer)
 	params := market2.PublishStorageDealsParams{Deals: deals}
 	err := params.MarshalCBOR(buf)
 	if err != nil {
 		panic(err)
 	}
+
 	return &types.Message{
 		Params: buf.Bytes(),
 	}, nil
@@ -310,15 +377,28 @@ func (mapi *CurrentDealInfoMockAPI) StateSearchMsg(ctx context.Context, c cid.Ci
 }
 
 func (mapi *CurrentDealInfoMockAPI) StateNetworkVersion(ctx context.Context, tok TipSetToken) (network.Version, error) {
-	return network.Version0, nil
+	return mapi.Version, nil
 }
 
-func makePublishDealsReturnBytes(t *testing.T, dealIDs []abi.DealID) []byte {
+func makePublishDealsReturnBytesOldVersion(t *testing.T, dealIDs []abi.DealID) []byte {
 	buf := new(bytes.Buffer)
 	dealsReturn := market0.PublishStorageDealsReturn{
 		IDs: dealIDs,
 	}
 	err := dealsReturn.MarshalCBOR(buf)
 	require.NoError(t, err)
+
+	return buf.Bytes()
+}
+
+func makePublishDealsReturn(t *testing.T, dealIDs []abi.DealID, validIdxs []uint64) []byte {
+	buf := new(bytes.Buffer)
+	dealsReturn := market7.PublishStorageDealsReturn{
+		IDs:        dealIDs,
+		ValidDeals: bitfield.NewFromSet(validIdxs),
+	}
+	err := dealsReturn.MarshalCBOR(buf)
+	require.NoError(t, err)
+
 	return buf.Bytes()
 }

markets/storageadapter/client.go

@@ -231,13 +231,13 @@ func (c *ClientNodeAdapter) ValidatePublishedDeal(ctx context.Context, deal stor
 		return 0, xerrors.Errorf("getting dealIDs: %w", err)
 	}
 
-	if dealIdx >= len(dealIDs) {
+	if dealIdx >= len(params.Deals) {
 		return 0, xerrors.Errorf(
 			"deal index %d out of bounds of deals (len %d) in publish deals message %s",
-			dealIdx, len(dealIDs), pubmsg.Cid())
+			dealIdx, len(params.Deals), pubmsg.Cid())
 	}
 
-	valid, err := res.IsDealValid(uint64(dealIdx))
+	valid, outIdx, err := res.IsDealValid(uint64(dealIdx))
 	if err != nil {
 		return 0, xerrors.Errorf("determining deal validity: %w", err)
 	}
@@ -246,7 +246,7 @@ func (c *ClientNodeAdapter) ValidatePublishedDeal(ctx context.Context, deal stor
 		return 0, xerrors.New("deal was invalid at publication")
 	}
 
-	return dealIDs[dealIdx], nil
+	return dealIDs[outIdx], nil
 }
 
 var clientOverestimation = struct {