Merge pull request #9134 from ellemouton/checkPayAddrBeforeDeref

routerrpc: check payaddr before using for probing

Author: Roasbeef

docs/release-notes/release-notes-0.19.0.md

@@ -23,6 +23,10 @@
   propagate mission control and debug level config values to the main LND config
   struct so that the GetDebugInfo response is accurate.
 
+* [Fix a bug](https://github.com/lightningnetwork/lnd/pull/9134) that would 
+  cause a nil pointer dereference during the probing of a payment request that 
+  does not contain a payment address.
+
 # New Features
 ## Functional Enhancements
 ## RPC Additions
@@ -69,5 +73,6 @@
 # Contributors (Alphabetical Order)
 
 * CharlieZKSmith
+* Elle Mouton
 * Pins
 * Ziggie

feature/default_sets.go

@@ -92,7 +92,4 @@ var defaultSetDesc = setDesc{
 		SetInit:    {}, // I
 		SetNodeAnn: {}, // N
 	},
-	lnwire.Bolt11BlindedPathsOptional: {
-		SetInvoice: {}, // I
-	},
 }

invoices/modification_interceptor.go

@@ -66,6 +66,9 @@ func (s *safeCallback) Exec(req HtlcModifyRequest) (*HtlcModifyResponse,
 // settle an invoice, enabling a subscribed client to modify certain aspects of
 // those HTLCs.
 type HtlcModificationInterceptor struct {
+	started atomic.Bool
+	stopped atomic.Bool
+
 	// callback is the wrapped client callback function that is called when
 	// an invoice is intercepted. This function gives the client the ability
 	// to determine how the invoice should be settled.
@@ -79,6 +82,7 @@ type HtlcModificationInterceptor struct {
 func NewHtlcModificationInterceptor() *HtlcModificationInterceptor {
 	return &HtlcModificationInterceptor{
 		callback: &safeCallback{},
+		quit:     make(chan struct{}),
 	}
 }
 
@@ -163,11 +167,19 @@ func (s *HtlcModificationInterceptor) RegisterInterceptor(
 
 // Start starts the service.
 func (s *HtlcModificationInterceptor) Start() error {
+	if !s.started.CompareAndSwap(false, true) {
+		return nil
+	}
+
 	return nil
 }
 
 // Stop stops the service.
 func (s *HtlcModificationInterceptor) Stop() error {
+	if !s.stopped.CompareAndSwap(false, true) {
+		return nil
+	}
+
 	close(s.quit)
 
 	return nil

lnrpc/routerrpc/router_backend.go

@@ -16,6 +16,7 @@ import (
 	sphinx "github.com/lightningnetwork/lightning-onion"
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/feature"
+	"github.com/lightningnetwork/lnd/fn"
 	"github.com/lightningnetwork/lnd/htlcswitch"
 	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lntypes"
@@ -1011,7 +1012,7 @@ func (r *RouterBackend) extractIntentFromSendRequest(
 			if len(rpcPayReq.PaymentAddr) > 0 {
 				var addr [32]byte
 				copy(addr[:], rpcPayReq.PaymentAddr)
-				payAddr = &addr
+				payAddr = fn.Some(addr)
 			}
 		} else {
 			err = payIntent.SetPaymentHash(*payReq.PaymentHash)
@@ -1128,7 +1129,7 @@ func (r *RouterBackend) extractIntentFromSendRequest(
 			} else {
 				copy(payAddr[:], rpcPayReq.PaymentAddr)
 			}
-			payIntent.PaymentAddr = &payAddr
+			payIntent.PaymentAddr = fn.Some(payAddr)
 
 			// Generate random SetID and root share.
 			var setID [32]byte
@@ -1167,7 +1168,7 @@ func (r *RouterBackend) extractIntentFromSendRequest(
 				var payAddr [32]byte
 				copy(payAddr[:], rpcPayReq.PaymentAddr)
 
-				payIntent.PaymentAddr = &payAddr
+				payIntent.PaymentAddr = fn.Some(payAddr)
 			}
 		}
 

lnrpc/routerrpc/router_server.go

@@ -528,11 +528,16 @@ func (s *Server) probePaymentRequest(ctx context.Context, paymentRequest string,
 		AmtMsat:          amtMsat,
 		PaymentHash:      paymentHash[:],
 		FeeLimitSat:      routeFeeLimitSat,
-		PaymentAddr:      payReq.PaymentAddr[:],
 		FinalCltvDelta:   int32(payReq.MinFinalCLTVExpiry()),
 		DestFeatures:     MarshalFeatures(payReq.Features),
 	}
 
+	// If the payment addresses is specified, then we'll also populate that
+	// now as well.
+	payReq.PaymentAddr.WhenSome(func(addr [32]byte) {
+		copy(probeRequest.PaymentAddr, addr[:])
+	})
+
 	hints := payReq.RouteHints
 
 	// If the hints don't indicate an LSP then chances are that our probe
@@ -1453,12 +1458,12 @@ func (s *Server) BuildRoute(_ context.Context,
 		outgoingChan = &req.OutgoingChanId
 	}
 
-	var payAddr *[32]byte
+	var payAddr fn.Option[[32]byte]
 	if len(req.PaymentAddr) != 0 {
 		var backingPayAddr [32]byte
 		copy(backingPayAddr[:], req.PaymentAddr)
 
-		payAddr = &backingPayAddr
+		payAddr = fn.Some(backingPayAddr)
 	}
 
 	if req.FinalCltvDelta == 0 {

routing/integrated_routing_context_test.go

@@ -187,7 +187,7 @@ func (c *integratedRoutingContext) testPayment(maxParts uint32,
 		FinalCLTVDelta: uint16(c.finalExpiry),
 		FeeLimit:       lnwire.MaxMilliSatoshi,
 		Target:         c.target.pubkey,
-		PaymentAddr:    &paymentAddr,
+		PaymentAddr:    fn.Some(paymentAddr),
 		DestFeatures: lnwire.NewFeatureVector(
 			baseFeatureBits, lnwire.Features,
 		),

routing/pathfind.go

@@ -111,7 +111,7 @@ type finalHopParams struct {
 	cltvDelta uint16
 
 	records     record.CustomSet
-	paymentAddr *[32]byte
+	paymentAddr fn.Option[[32]byte]
 
 	// metadata is additional data that is sent along with the payment to
 	// the payee.
@@ -226,20 +226,17 @@ func newRoute(sourceVertex route.Vertex,
 			// If we're attaching a payment addr but the receiver
 			// doesn't support both TLV and payment addrs, fail.
 			payAddr := supports(lnwire.PaymentAddrOptional)
-			if !payAddr && finalHop.paymentAddr != nil {
+			if !payAddr && finalHop.paymentAddr.IsSome() {
 				return nil, errors.New("cannot attach " +
 					"payment addr")
 			}
 
 			// Otherwise attach the mpp record if it exists.
 			// TODO(halseth): move this to payment life cycle,
 			// where AMP options are set.
-			if finalHop.paymentAddr != nil {
-				mpp = record.NewMPP(
-					finalHop.totalAmt,
-					*finalHop.paymentAddr,
-				)
-			}
+			finalHop.paymentAddr.WhenSome(func(addr [32]byte) {
+				mpp = record.NewMPP(finalHop.totalAmt, addr)
+			})
 
 			metadata = finalHop.metadata
 
@@ -452,7 +449,7 @@ type RestrictParams struct {
 	// PaymentAddr is a random 32-byte value generated by the receiver to
 	// mitigate probing vectors and payment sniping attacks on overpaid
 	// invoices.
-	PaymentAddr *[32]byte
+	PaymentAddr fn.Option[[32]byte]
 
 	// Amp signals to the pathfinder that this payment is an AMP payment
 	// and therefore it needs to account for additional AMP data in the
@@ -608,7 +605,7 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig,
 	// checking that it supports the features we need. If the caller has a
 	// payment address to attach, check that our destination feature vector
 	// supports them.
-	if r.PaymentAddr != nil &&
+	if r.PaymentAddr.IsSome() &&
 		!features.HasFeature(lnwire.PaymentAddrOptional) {
 
 		return nil, 0, errNoPaymentAddr
@@ -1435,9 +1432,9 @@ func lastHopPayloadSize(r *RestrictParams, finalHtlcExpiry int32,
 	}
 
 	var mpp *record.MPP
-	if r.PaymentAddr != nil {
-		mpp = record.NewMPP(amount, *r.PaymentAddr)
-	}
+	r.PaymentAddr.WhenSome(func(addr [32]byte) {
+		mpp = record.NewMPP(amount, addr)
+	})
 
 	var amp *record.AMP
 	if r.Amp != nil {

routing/pathfind_test.go

@@ -1367,7 +1367,7 @@ func TestNewRoute(t *testing.T) {
 		// overwrite the final hop's feature vector in the graph.
 		destFeatures *lnwire.FeatureVector
 
-		paymentAddr *[32]byte
+		paymentAddr fn.Option[[32]byte]
 
 		// metadata is the payment metadata to attach to the route.
 		metadata []byte
@@ -1446,7 +1446,7 @@ func TestNewRoute(t *testing.T) {
 			// a fee to receive the payment.
 			name:          "two hop single shot mpp",
 			destFeatures:  tlvPayAddrFeatures,
-			paymentAddr:   &testPaymentAddr,
+			paymentAddr:   fn.Some(testPaymentAddr),
 			paymentAmount: 100000,
 			hops: []*models.CachedEdgePolicy{
 				createHop(0, 1000, 1000000, 10),
@@ -1911,7 +1911,7 @@ func runDestPaymentAddr(t *testing.T, useCache bool) {
 	luoji := ctx.keyFromAlias("luoji")
 
 	// Add payment address w/o any invoice features.
-	ctx.restrictParams.PaymentAddr = &[32]byte{1}
+	ctx.restrictParams.PaymentAddr = fn.Some([32]byte{1})
 
 	// Add empty destination features. This should cause us to fail, since
 	// this overrides anything in the graph.
@@ -2955,7 +2955,7 @@ func runInboundFees(t *testing.T, useCache bool) {
 	ctx := newPathFindingTestContext(t, useCache, testChannels, "a")
 
 	payAddr := [32]byte{1}
-	ctx.restrictParams.PaymentAddr = &payAddr
+	ctx.restrictParams.PaymentAddr = fn.Some(payAddr)
 	ctx.restrictParams.DestFeatures = tlvPayAddrFeatures
 
 	const (
@@ -2974,7 +2974,7 @@ func runInboundFees(t *testing.T, useCache bool) {
 			amt:         paymentAmt,
 			cltvDelta:   finalHopCLTV,
 			records:     nil,
-			paymentAddr: &payAddr,
+			paymentAddr: fn.Some(payAddr),
 			totalAmt:    paymentAmt,
 		},
 		nil,
@@ -3469,7 +3469,7 @@ func TestLastHopPayloadSize(t *testing.T) {
 		{
 			name: "Non blinded final hop",
 			restrictions: &RestrictParams{
-				PaymentAddr:       paymentAddr,
+				PaymentAddr:       fn.Some(*paymentAddr),
 				DestCustomRecords: customRecords,
 				Metadata:          metadata,
 				Amp:               ampOptions,
@@ -3501,12 +3501,10 @@ func TestLastHopPayloadSize(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			var mpp *record.MPP
-			if tc.restrictions.PaymentAddr != nil {
-				mpp = record.NewMPP(
-					tc.amount, *tc.restrictions.PaymentAddr,
-				)
-			}
+			mpp := fn.MapOptionZ(tc.restrictions.PaymentAddr,
+				func(addr [32]byte) *record.MPP {
+					return record.NewMPP(tc.amount, addr)
+				})
 
 			// In case it's an AMP payment we use the max AMP record
 			// size to estimate the final hop size.

routing/payment_session.go

@@ -344,7 +344,7 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi,
 			// record. If it has a blinded path though, then we
 			// can split. Split payments to blinded paths won't have
 			// MPP records.
-			if p.payment.PaymentAddr == nil &&
+			if p.payment.PaymentAddr.IsNone() &&
 				p.payment.BlindedPathSet == nil {
 
 				p.log.Debugf("not splitting because payment " +

routing/router.go

@@ -862,7 +862,7 @@ type LightningPayment struct {
 	// PaymentAddr is the payment address specified by the receiver. This
 	// field should be a random 32-byte nonce presented in the receiver's
 	// invoice to prevent probing of the destination.
-	PaymentAddr *[32]byte
+	PaymentAddr fn.Option[[32]byte]
 
 	// PaymentRequest is an optional payment request that this payment is
 	// attempting to complete.
@@ -1063,9 +1063,10 @@ func (r *ChannelRouter) PreparePayment(payment *LightningPayment) (
 	switch {
 	// If this is an AMP payment, we'll use the AMP shard tracker.
 	case payment.amp != nil:
+		addr := payment.PaymentAddr.UnwrapOr([32]byte{})
 		shardTracker = amp.NewShardTracker(
-			payment.amp.RootShare, payment.amp.SetID,
-			*payment.PaymentAddr, payment.Amount,
+			payment.amp.RootShare, payment.amp.SetID, addr,
+			payment.Amount,
 		)
 
 	// Otherwise we'll use the simple tracker that will map each attempt to
@@ -1367,8 +1368,8 @@ func (e ErrNoChannel) Error() string {
 // outgoing channel, use the outgoingChan parameter.
 func (r *ChannelRouter) BuildRoute(amt fn.Option[lnwire.MilliSatoshi],
 	hops []route.Vertex, outgoingChan *uint64, finalCltvDelta int32,
-	payAddr *[32]byte, firstHopBlob fn.Option[[]byte]) (*route.Route,
-	error) {
+	payAddr fn.Option[[32]byte], firstHopBlob fn.Option[[]byte]) (
+	*route.Route, error) {
 
 	log.Tracef("BuildRoute called: hopsCount=%v, amt=%v", len(hops), amt)