Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

splice: minor code cleanups #6729

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Sep 27, 2023

A few little stylistic things were bugging me when reading through the splice code so I cleaned them up.

ChangeLog-None

@ddustin ddustin added this to the v23.11 milestone Sep 27, 2023
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa99d92

A few little stylistic things were bugging me when reading through the splice code so I cleaned them up.

ChangeLog-None
@rustyrussell rustyrussell force-pushed the ddustin/splice_cleanup branch from fa99d92 to 25f7b71 Compare October 2, 2023 03:56
@rustyrussell
Copy link
Contributor

Trivial rebase on master for CI...

In this case, we were allocating off NULL, which meant a leak:

```
 MEMLEAK: 0x565086722e98
   label=channeld/channeld.c:3433:struct inflight
   backtrace:
     ccan/ccan/tal/tal.c:477 (tal_alloc_)
     channeld/channeld.c:3433 (inflights_new)
     channeld/channeld.c:3573 (splice_accepter)
     channeld/channeld.c:4145 (peer_in)
     channeld/channeld.c:6051 (main)
   parents:
```

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor

Fixed memleak due to tal off NULL....

We steal it onto "peer" where we should steal it onto the inflight:

```
label=struct bitcoin_tx
backtrace:
  ccan/ccan/tal/tal.c:477 (tal_alloc_)
  bitcoin/tx.c:612 (clone_bitcoin_tx)
  channeld/channeld.c:2163 (handle_peer_commit_sig)
  channeld/channeld.c:2191 (handle_peer_commit_sig)
  channeld/channeld.c:2831 (interactive_send_commitments)
  channeld/channeld.c:3814 (splice_initiator_user_finalized)
  channeld/channeld.c:3882 (splice_initiator_user_update)
  channeld/channeld.c:5651 (req_in)
  channeld/channeld.c:6044 (main)
  ../sysdeps/nptl/libc_start_call_main.h:58 (__libc_start_call_main)
  ../csu/libc-start.c:360 (__libc_start_main_impl)
parents:
  struct peer
```

Signed-off-by: Rusty Russell <[email protected]>
@ddustin
Copy link
Collaborator Author

ddustin commented Oct 3, 2023

Thanks for the memory fixes 🙏. Looks like this was triggered by removing the ugly way of explicitly free'ing: 25f7b71#diff-b1299b24697159a06e7b426c5086c36df8e48db15739a0e301d53a9a21149722L770

	/* valgrind complains last_tx is leaked so we explicitly free it
	 * DTODO: investigate this more. */
	for (size_t i = 0; i < tal_count(peer->splice_state->inflights); i++) {
		inflight = peer->splice_state->inflights[i];
		tal_free(inflight->last_tx);
		tal_free(inflight);
	}

@rustyrussell rustyrussell merged commit 11df13e into ElementsProject:master Oct 3, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants