Merge pull request #627 from ethereumproject/fix/atxi-pagination-slic…

…e-oob

fix atxi pagination OoB possibility

cmd/geth/build_atxi_cmd.go

@@ -1,11 +1,12 @@
 package main
 
 import (
+	"math"
+
 	"github.com/ethereumproject/go-ethereum/core"
 	"github.com/ethereumproject/go-ethereum/ethdb"
 	"github.com/ethereumproject/go-ethereum/logger/glog"
 	"gopkg.in/urfave/cli.v1"
-	"math"
 )
 
 var buildAddrTxIndexCommand = cli.Command{

core/atxi.go

@@ -5,22 +5,24 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
-	"github.com/ethereumproject/go-ethereum/common"
-	"github.com/ethereumproject/go-ethereum/core/types"
-	"github.com/ethereumproject/go-ethereum/ethdb"
-	"github.com/ethereumproject/go-ethereum/logger"
-	"github.com/ethereumproject/go-ethereum/logger/glog"
 	"math"
 	"os"
 	"os/signal"
 	"sort"
 	"strings"
 	"syscall"
 	"time"
+
+	"github.com/ethereumproject/go-ethereum/common"
+	"github.com/ethereumproject/go-ethereum/core/types"
+	"github.com/ethereumproject/go-ethereum/ethdb"
+	"github.com/ethereumproject/go-ethereum/logger"
+	"github.com/ethereumproject/go-ethereum/logger/glog"
 )
 
 var (
 	errAtxiNotEnabled = errors.New("atxi not intialized")
+	errAtxiInvalidUse = errors.New("invalid parameters passed to ATXI")
 
 	txAddressIndexPrefix = []byte("atx-")
 	txAddressBookmarkKey = []byte("ATXIBookmark")
@@ -288,18 +290,33 @@ func (bc *BlockChain) GetATXIBuildProgress() (*AtxiProgressT, error) {
 
 // GetAddrTxs gets the indexed transactions for a given account address.
 // 'reverse' means "oldest first"
-func GetAddrTxs(db ethdb.Database, address common.Address, blockStartN uint64, blockEndN uint64, direction string, kindof string, paginationStart int, paginationEnd int, reverse bool) []string {
+func GetAddrTxs(db ethdb.Database, address common.Address, blockStartN uint64, blockEndN uint64, direction string, kindof string, paginationStart int, paginationEnd int, reverse bool) (txs []string, err error) {
+	errWithReason := func(e error, s string) error {
+		return fmt.Errorf("%v: %s", e, s)
+	}
+
+	// validate params
 	if len(direction) > 0 && !strings.Contains("btf", direction[:1]) {
-		glog.Fatal("Address transactions list signature requires direction param to be empty string or [b|t|f] prefix (eg. both, to, or from)")
+		err = errWithReason(errAtxiInvalidUse, "Address transactions list signature requires direction param to be empty string or [b|t|f] prefix (eg. both, to, or from)")
+		return
 	}
 	if len(kindof) > 0 && !strings.Contains("bsc", kindof[:1]) {
-		glog.Fatal("Address transactions list signature requires 'kind of' param to be empty string or [s|c] prefix (eg. both, standard, or contract)")
+		err = errWithReason(errAtxiInvalidUse, "Address transactions list signature requires 'kind of' param to be empty string or [s|c] prefix (eg. both, standard, or contract)")
+		return
+	}
+	if paginationStart > 0 && paginationEnd > 0 && paginationStart > paginationEnd {
+		err = errWithReason(errAtxiInvalidUse, "Pagination start must be less than or equal to pagination end params")
+		return
+	}
+	if paginationStart < 0 {
+		paginationStart = 0
 	}
 
 	// Have to cast to LevelDB to use iterator. Yuck.
 	ldb, ok := db.(*ethdb.LDBDatabase)
 	if !ok {
-		return nil
+		err = errWithReason(errors.New("internal interface error; please file a bug report"), "could not cast eth db to level db")
+		return nil, nil
 	}
 
 	// This will be the returnable.
@@ -351,8 +368,9 @@ func GetAddrTxs(db ethdb.Database, address common.Address, blockStartN uint64, b
 		atxis = append(atxis, atxi{blockN: bn, tx: tx})
 	}
 	it.Release()
-	if e := it.Error(); e != nil {
-		glog.Fatalln(e)
+	err = it.Error()
+	if err != nil {
+		return
 	}
 
 	handleSorting := func(s sortableAtxis) sortableAtxis {
@@ -365,16 +383,16 @@ func GetAddrTxs(db ethdb.Database, address common.Address, blockStartN uint64, b
 				s[i], s[j] = s[j], s[i]
 			}
 		}
-		if paginationStart < 0 {
-			paginationStart = 0
+		if paginationStart > len(s) {
+			paginationStart = len(s)
 		}
-		if paginationEnd < 0 {
+		if paginationEnd < 0 || paginationEnd > len(s) {
 			paginationEnd = len(s)
 		}
 		return s[paginationStart:paginationEnd]
 	}
-
-	return handleSorting(atxis).TxStrings()
+	txs = handleSorting(atxis).TxStrings()
+	return
 }
 
 // RmAddrTx removes all atxi indexes for a given tx in case of a transaction removal, eg.

core/blockchain_test.go

@@ -982,23 +982,30 @@ func TestFastVsFullChainsATXI(t *testing.T) {
 			}
 		}
 
-		out := GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
+		out, _ := GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
 		if len(out) != 3 {
 			t.Errorf("[%d] got: %v, want: %v", i, len(out), 3)
 		}
-		out = GetAddrTxs(db, addr1, 0, 0, "from", "", -1, -1, false)
+
+		// method should return an error if pagination params are invalid
+		_, err = GetAddrTxs(db, addr1, 0, 0, "", "", 2, 1, false)
+		if err == nil {
+			t.Errorf("[%d] got: %v, want: %v", i, err, errAtxiInvalidUse)
+		}
+
+		out, _ = GetAddrTxs(db, addr1, 0, 0, "from", "", -1, -1, false)
 		if len(out) != 2 {
 			t.Errorf("[%d] got: %v, want: %v", i, len(out), 2)
 		}
-		out = GetAddrTxs(db, addr1, 0, 0, "to", "", -1, -1, false)
+		out, _ = GetAddrTxs(db, addr1, 0, 0, "to", "", -1, -1, false)
 		if len(out) != 1 {
 			t.Errorf("[%d] got: %v, want: %v", i, len(out), 1)
 		}
-		out = GetAddrTxs(db, addr2, 0, 0, "", "", -1, -1, false)
+		out, _ = GetAddrTxs(db, addr2, 0, 0, "", "", -1, -1, false)
 		if len(out) != 3 {
 			t.Errorf("[%d] got: %v, want: %v", i, len(out), 3)
 		}
-		out = GetAddrTxs(db, addr2, 3, 3, "", "", -1, -1, false)
+		out, _ = GetAddrTxs(db, addr2, 3, 3, "", "", -1, -1, false)
 		if len(out) != 1 {
 			t.Errorf("[%d] got: %v, want: %v", i, len(out), 1)
 		}
@@ -1076,14 +1083,14 @@ func TestRmAddrTx(t *testing.T) {
 		t.Fatalf("failed to process block %d: %v", res.Index, res.Error)
 	}
 
-	out := GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
+	out, _ := GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
 	if len(out) != 3 {
 		t.Errorf("got: %v, want: %v", len(out), 3)
 	}
 	if err := RmAddrTx(db, t1); err != nil {
 		t.Fatal(err)
 	}
-	out = GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
+	out, _ = GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
 	if len(out) != 2 {
 		t.Errorf("got: %v, want: %v", len(out), 2)
 	}
@@ -1363,9 +1370,9 @@ func testChainTxReorgs(t *testing.T, db ethdb.Database, withATXI bool) {
 	if !withATXI {
 		return
 	}
-	txsh1 := GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
-	txsh2 := GetAddrTxs(db, addr2, 0, 0, "", "", -1, -1, false)
-	txsh3 := GetAddrTxs(db, addr3, 0, 0, "", "", -1, -1, false)
+	txsh1, _ := GetAddrTxs(db, addr1, 0, 0, "", "", -1, -1, false)
+	txsh2, _ := GetAddrTxs(db, addr2, 0, 0, "", "", -1, -1, false)
+	txsh3, _ := GetAddrTxs(db, addr3, 0, 0, "", "", -1, -1, false)
 
 	allAtxis := txsh1
 	allAtxis = append(allAtxis, txsh2...)

core/database_util_test.go

@@ -395,13 +395,13 @@ func TestAddrTxStorage(t *testing.T) {
 		t.Errorf("want: %v, got: %v", 7, count)
 	}
 
-	out := GetAddrTxs(db, from2, 0, 0, "", "", -1, -1, false)
+	out, _ := GetAddrTxs(db, from2, 0, 0, "", "", -1, -1, false)
 	if len(out) != 3 {
 		t.Errorf("want: %v, got: %v", 3, len(out))
 	}
 
 	// Test pagination and reverse
-	outReverse := GetAddrTxs(db, from2, 0, 0, "", "", -1, -1, true)
+	outReverse, _ := GetAddrTxs(db, from2, 0, 0, "", "", -1, -1, true)
 	if len(outReverse) != 3 {
 		t.Errorf("want: %v, got: %v", 3, len(outReverse))
 	}
@@ -410,21 +410,21 @@ func TestAddrTxStorage(t *testing.T) {
 		t.Errorf("got: %v, want: %v", outReverse, out)
 	}
 	// pagination
-	outPag := GetAddrTxs(db, from2, 0, 0, "", "", 1, -1, false)
+	outPag, _ := GetAddrTxs(db, from2, 0, 0, "", "", 1, -1, false)
 	if len(outPag) != 2 {
 		t.Errorf("got: %v, want: %v", len(outPag), 2)
 	}
 
-	out = GetAddrTxs(db, from2, 0, 0, "", "c", -1, -1, false)
+	out, _ = GetAddrTxs(db, from2, 0, 0, "", "c", -1, -1, false)
 	if len(out) != 1 {
 		t.Errorf("got: %v, want: %v", len(out), 1)
 	}
-	out = GetAddrTxs(db, common.Address{}, 0, 0, "", "", -1, -1, false)
+	out, _ = GetAddrTxs(db, common.Address{}, 0, 0, "", "", -1, -1, false)
 	if len(out) != 1 {
 		t.Errorf("got: %v, want: %v", len(out), 1)
 	}
 
-	out = GetAddrTxs(db, from1, 314, 314, "", "", -1, -1, false)
+	out, _ = GetAddrTxs(db, from1, 314, 314, "", "", -1, -1, false)
 	if len(out) != 1 {
 		t.Errorf("want: %v, got: %v", 1, len(out))
 	} else {
@@ -447,7 +447,7 @@ func TestAddrTxStorage(t *testing.T) {
 		}
 	}
 
-	out = GetAddrTxs(db, from2to, 314, 314, "to", "", -1, -1, false)
+	out, _ = GetAddrTxs(db, from2to, 314, 314, "to", "", -1, -1, false)
 	if len(out) != 1 {
 		t.Errorf("want: %v, got: %v", 1, len(out))
 	} else {
@@ -474,7 +474,7 @@ func TestAddrTxStorage(t *testing.T) {
 			t.Errorf("got: %v, want: %v", f, from2)
 		}
 	}
-	out = GetAddrTxs(db, from2to, 314, 314, "from", "", -1, -1, false)
+	out, _ = GetAddrTxs(db, from2to, 314, 314, "from", "", -1, -1, false)
 	if len(out) != 0 {
 		t.Errorf("want: %v, got: %v", 0, len(out))
 	}

eth/api.go

@@ -1731,7 +1731,10 @@ func (api *PublicGethAPI) GetAddressTransactions(address common.Address, blockSt
 		blockEndN = 0
 	}
 
-	list = core.GetAddrTxs(atxi.Db, address, blockStartN, uint64(blockEndN.Int64()), toOrFrom, txKindOf, pagStart, pagEnd, reverse)
+	list, err = core.GetAddrTxs(atxi.Db, address, blockStartN, uint64(blockEndN.Int64()), toOrFrom, txKindOf, pagStart, pagEnd, reverse)
+	if err != nil {
+		return
+	}
 
 	// Since list is a slice, it can be nil, which returns 'null'.
 	// Should return empty 'array' if no txs found.