Skip to content

Commit

Permalink
Add safety checks for address pool access
Browse files Browse the repository at this point in the history 
Address pool access now has checks to ensure that the address
pool exists and has been initialized before calls accessing its
functions or internals.
  • Loading branch information
@cjepson
cjepson committed Apr 1, 2016
1 parent 5141dfa commit 125bbdd
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 36 deletions.
48 changes: 48 additions & 0 deletions wallet/addresspool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,42 @@ func (w *Wallet) CloseAddressPools() {
return
}

// CheckAddressPoolsInitialized checks to make sure an address pool exists
// that that one can safely access functions and internal memory such as
// mutexes.
func (w *Wallet) CheckAddressPoolsInitialized(account uint32) error {
if w.addrPools[account] == nil {
return fmt.Errorf("Address pools for account %v "+
"are undeclared", account)
}
if w.addrPools[account].external == nil {
return fmt.Errorf("External address pool for "+
"account %v is undeclared", account)
}
if w.addrPools[account].internal == nil {
return fmt.Errorf("Internal address pool for "+
"account %v is undeclared", account)
}
if !w.addrPools[account].external.started {
return fmt.Errorf("External address pool for the "+
"account %v is uninitialized", account)
}
if !w.addrPools[account].internal.started {
return fmt.Errorf("Internal address pool for the "+
"account %v is uninitialized", account)
}

return nil
}

// GetNewAddressExternal is the exported function that gets a new external address
// for the default account from the external address mempool.
func (w *Wallet) GetNewAddressExternal() (dcrutil.Address, error) {
err := w.CheckAddressPoolsInitialized(waddrmgr.DefaultAccountNum)
if err != nil {
return nil, err
}

w.addrPools[waddrmgr.DefaultAccountNum].external.mutex.Lock()
defer w.addrPools[waddrmgr.DefaultAccountNum].external.mutex.Unlock()
address, err :=
Expand All @@ -283,6 +316,11 @@ func (w *Wallet) GetNewAddressExternal() (dcrutil.Address, error) {
// GetNewAddressInternal is the exported function that gets a new internal address
// for the default account from the internal address mempool.
func (w *Wallet) GetNewAddressInternal() (dcrutil.Address, error) {
err := w.CheckAddressPoolsInitialized(waddrmgr.DefaultAccountNum)
if err != nil {
return nil, err
}

w.addrPools[waddrmgr.DefaultAccountNum].internal.mutex.Lock()
defer w.addrPools[waddrmgr.DefaultAccountNum].internal.mutex.Unlock()
address, err :=
Expand All @@ -296,6 +334,11 @@ func (w *Wallet) GetNewAddressInternal() (dcrutil.Address, error) {
// NewAddress returns the next external chained address for a wallet given some
// account.
func (w *Wallet) NewAddress(account uint32) (dcrutil.Address, error) {
err := w.CheckAddressPoolsInitialized(account)
if err != nil {
return nil, err
}

// Get next address from wallet.
addr, err := w.addrPools[account].external.GetNewAddress()
if err != nil {
Expand Down Expand Up @@ -328,6 +371,11 @@ func (w *Wallet) NewAddress(account uint32) (dcrutil.Address, error) {

// NewChangeAddress returns a new change address for a wallet.
func (w *Wallet) NewChangeAddress(account uint32) (dcrutil.Address, error) {
err := w.CheckAddressPoolsInitialized(account)
if err != nil {
return nil, err
}

// Get next chained change address from wallet for account.
addr, err := w.addrPools[account].internal.GetNewAddress()
if err != nil {
Expand Down
88 changes: 52 additions & 36 deletions wallet/createtx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,14 +351,18 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut, account uint32, minconf int3
// the default account to create change.
var pool *addressPool
if account == waddrmgr.ImportedAddrAccount {
err := w.CheckAddressPoolsInitialized(waddrmgr.DefaultAccountNum)
if err != nil {
return nil, err
}
pool = w.addrPools[waddrmgr.DefaultAccountNum].internal
} else {
err := w.CheckAddressPoolsInitialized(account)
if err != nil {
return nil, err
}
pool = w.addrPools[account].internal
}
if pool == nil {
log.Errorf("tried to use uninitialized pool for acct %v "+
"when attempting to make a transaction", account)
}
txSucceeded := false
pool.mutex.Lock()
defer pool.mutex.Unlock()
Expand Down Expand Up @@ -449,19 +453,28 @@ func constructMultiSigScript(keys []dcrutil.AddressSecpPubKey,
func (w *Wallet) txToMultisig(account uint32, amount dcrutil.Amount,
pubkeys []*dcrutil.AddressSecpPubKey, nRequired int8,
minconf int32) (*CreatedTx, dcrutil.Address, []byte, error) {
txToMultisigError :=
func(err error) (*CreatedTx, dcrutil.Address, []byte, error) {
return nil, nil, nil, err
}

// Initialize the address pool for use. If we
// are using an imported account, loopback to
// the default account to create change.
var pool *addressPool
if account == waddrmgr.ImportedAddrAccount {
err := w.CheckAddressPoolsInitialized(waddrmgr.DefaultAccountNum)
if err != nil {
return txToMultisigError(err)
}
pool = w.addrPools[waddrmgr.DefaultAccountNum].internal
} else {
err := w.CheckAddressPoolsInitialized(account)
if err != nil {
return txToMultisigError(err)
}
pool = w.addrPools[account].internal
}
if pool == nil {
log.Errorf("tried to use uninitialized pool for acct %v "+
"when attempting to make a transaction", account)
}
txSucceeded := false
pool.mutex.Lock()
defer pool.mutex.Unlock()
Expand All @@ -474,34 +487,29 @@ func (w *Wallet) txToMultisig(account uint32, amount dcrutil.Amount,
}()
addrFunc := pool.GetNewAddress

errorOut :=
func(err error) (*CreatedTx, dcrutil.Address, []byte, error) {
return nil, nil, nil, err
}

chainClient, err := w.requireChainClient()
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}

isReorganizing, _ := chainClient.GetReorganizing()
if isReorganizing {
return errorOut(ErrBlockchainReorganizing)
return txToMultisigError(err)
}

// Address manager must be unlocked to compose transaction. Grab
// the unlock if possible (to prevent future unlocks), or return the
// error if already locked.
heldUnlock, err := w.HoldUnlock()
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
defer heldUnlock.Release()

// Get current block's height and hash.
bs, err := chainClient.BlockStamp()
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}

// Add in some extra for fees. TODO In the future, make a better
Expand All @@ -522,10 +530,10 @@ func (w *Wallet) txToMultisig(account uint32, amount dcrutil.Amount,
eligible, err := w.findEligibleOutputsAmount(account, minconf,
amountRequired, bs)
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
if eligible == nil {
return errorOut(
return txToMultisigError(
fmt.Errorf("Not enough funds to send to multisig address"))
}

Expand All @@ -549,27 +557,27 @@ func (w *Wallet) txToMultisig(account uint32, amount dcrutil.Amount,
totalOutput := dcrutil.Amount(0)
msScript, err := txscript.MultiSigScript(pubkeys, int(nRequired))
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
_, err = w.Manager.ImportScript(msScript, bs)
if err != nil {
// We don't care if we've already used this address.
if err.(waddrmgr.ManagerError).ErrorCode !=
waddrmgr.ErrDuplicateAddress {
return errorOut(err)
return txToMultisigError(err)
}
}
err = w.TxStore.InsertTxScript(msScript)
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
scAddr, err := dcrutil.NewAddressScriptHash(msScript, w.chainParams)
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
p2shScript, err := txscript.PayToAddrScript(scAddr)
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
txout := wire.NewTxOut(int64(amount), p2shScript)
msgtx.AddTxOut(txout)
Expand All @@ -586,43 +594,44 @@ func (w *Wallet) txToMultisig(account uint32, amount dcrutil.Amount,
feeEst := feeForSize(feeIncrement, feeSize)

if totalInput < amount+feeEst {
return errorOut(fmt.Errorf("Not enough funds to send to " +
return txToMultisigError(fmt.Errorf("Not enough funds to send to " +
"multisig address after accounting for fees"))
}
if totalInput > amount+feeEst {
changeAddr, err := addrFunc()
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}
change := totalInput - (amount + feeEst)
pkScript, err := txscript.PayToAddrScript(changeAddr)
if err != nil {
return errorOut(fmt.Errorf("cannot create txout script: %s", err))
return txToMultisigError(
fmt.Errorf("cannot create txout script: %s", err))
}
msgtx.AddTxOut(wire.NewTxOut(int64(change), pkScript))
}

if err = signMsgTx(msgtx, forSigning, w.Manager,
w.chainParams); err != nil {
return errorOut(err)
return txToMultisigError(err)
}

_, err = chainClient.SendRawTransaction(msgtx, false)
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}

// Request updates from dcrd for new transactions sent to this
// script hash address.
utilAddrs := make([]dcrutil.Address, 1)
utilAddrs[0] = scAddr
if err := chainClient.NotifyReceived(utilAddrs); err != nil {
return errorOut(err)
return txToMultisigError(err)
}

err = w.insertMultisigOutIntoTxMgr(msgtx, 0)
if err != nil {
return errorOut(err)
return txToMultisigError(err)
}

ctx := &CreatedTx{
Expand Down Expand Up @@ -680,20 +689,23 @@ func (w *Wallet) compressWallet(maxNumIns int, account uint32) (*chainhash.Hash,
return nil, err
}

// Initialize the address pool for use.
// Initialize the address pool for use. If we
// are using an imported account, loopback to
// the default account to create change.
var pool *addressPool
if account == waddrmgr.ImportedAddrAccount {
err := w.CheckAddressPoolsInitialized(waddrmgr.DefaultAccountNum)
if err != nil {
return nil, err
}
pool = w.addrPools[waddrmgr.DefaultAccountNum].internal
} else {
err := w.CheckAddressPoolsInitialized(account)
if err != nil {
return nil, err
}
pool = w.addrPools[account].internal
}
if pool == nil {
log.Errorf("tried to use uninitialized pool for acct %v "+
"when attempting to make a transaction", account)
}
txSucceeded := false
pool.mutex.Lock()
defer pool.mutex.Unlock()
Expand Down Expand Up @@ -798,6 +810,10 @@ func (w *Wallet) compressEligible(eligible []wtxmgr.Credit) error {

// Initialize the address pool for use.
var pool *addressPool
err = w.CheckAddressPoolsInitialized(waddrmgr.DefaultAccountNum)
if err != nil {
return err
}
pool = w.addrPools[waddrmgr.DefaultAccountNum].internal
if pool == nil {
log.Errorf("tried to use uninitialized pool for acct %v "+
Expand Down

0 comments on commit 125bbdd

Please sign in to comment.