From: Matt Corallo <git@bluematt.me>
Date: Tue, 26 Oct 2021 21:40:14 +0000 (+0000)
Subject: Fix a minor memory leak on PermanentFailure mon errs when sending
X-Git-Tag: v0.0.104~39^2
X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=refs%2Fheads%2F2021-10-no-payment-id-leaks;p=rust-lightning

Fix a minor memory leak on PermanentFailure mon errs when sending

If we send a payment and fail to update the first-hop channel state
with a `PermanentFailure` ChannelMonitorUpdateErr, we would have an
entry in our pending payments map, but possibly not return the
PaymentId back to the user to retry the payment, leading to a (rare
and relatively minor) memory leak.
---

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 763a293fa..444d443fa 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2075,6 +2075,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				Some(id) => id.clone(),
 			};
 
+			macro_rules! insert_outbound_payment {
+				() => {
+					let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
+						session_privs: HashSet::new(),
+						pending_amt_msat: 0,
+						pending_fee_msat: Some(0),
+						payment_hash: *payment_hash,
+						payment_secret: *payment_secret,
+						starting_block_height: self.best_block.read().unwrap().height(),
+						total_msat: total_value,
+					});
+					assert!(payment.insert(session_priv_bytes, path));
+				}
+			}
+
 			let channel_state = &mut *channel_lock;
 			if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
 				match {
@@ -2084,7 +2099,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					if !chan.get().is_live() {
 						return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
 					}
-					let send_res = break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
+					break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
 						htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
 							path: path.clone(),
 							session_priv: session_priv.clone(),
@@ -2093,20 +2108,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							payment_secret: payment_secret.clone(),
 							payee: payee.clone(),
 						}, onion_packet, &self.logger),
-					channel_state, chan);
-
-					let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
-						session_privs: HashSet::new(),
-						pending_amt_msat: 0,
-						pending_fee_msat: Some(0),
-						payment_hash: *payment_hash,
-						payment_secret: *payment_secret,
-						starting_block_height: self.best_block.read().unwrap().height(),
-						total_msat: total_value,
-					});
-					assert!(payment.insert(session_priv_bytes, path));
-
-					send_res
+					channel_state, chan)
 				} {
 					Some((update_add, commitment_signed, monitor_update)) => {
 						if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
@@ -2116,8 +2118,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							// is restored. Therefore, we must return an error indicating that
 							// it is unsafe to retry the payment wholesale, which we do in the
 							// send_payment check for MonitorUpdateFailed, below.
+							insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above.
 							return Err(APIError::MonitorUpdateFailed);
 						}
+						insert_outbound_payment!();
 
 						log_debug!(self.logger, "Sending payment along path resulted in a commitment_signed for channel {}", log_bytes!(chan.get().channel_id()));
 						channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
@@ -2132,7 +2136,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							},
 						});
 					},
-					None => {},
+					None => { insert_outbound_payment!(); },
 				}
 			} else { unreachable!(); }
 			return Ok(());
@@ -2249,6 +2253,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		if has_err && has_ok {
 			Err(PaymentSendFailure::PartialFailure(results))
 		} else if has_err {
+			// If we failed to send any paths, we shouldn't have inserted the new PaymentId into
+			// our `pending_outbound_payments` map at all.
+			debug_assert!(self.pending_outbound_payments.lock().unwrap().get(&payment_id).is_none());
 			Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
 		} else {
 			Ok(payment_id)