Avoid accessing legacy TLV fields after the TLV-stream read
#3622
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`legacy` fields in TLV stream reads may be used to read fields which are later moved into some other field using either `default_value` or `static_value` "reads". This works fine if the field supports copy semantics, however if it does not, the accessing of the field in `_decode_and_build` after the TLV stream read completes but before the struct is built results in a "use after move" error. Instead, here, we drop the attempt to hide unused variable warnings entirely, dropping the post-TLV-stream access to legacy variables, allowing their use in move semantics for later fields.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3622 +/- ##
==========================================
- Coverage 88.60% 88.59% -0.01%
==========================================
Files 151 151
Lines 118454 118454
Branches 118454 118454
==========================================
- Hits 104957 104949 -8
+ Misses 10985 10983 -2
- Partials 2512 2522 +10 ☔ View full report in Codecov by Sentry. |
valentinewallace
approved these changes
Feb 26, 2025
valentinewallace
merged commit 047bd61
into
lightningdevkit:main
24 of 26 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
legacyfields in TLV stream reads may be used to read fields which are later moved into some other field using eitherdefault_valueorstatic_value"reads". This works fine if the field supports copy semantics, however if it does not, the accessing of the field in_decode_and_buildafter the TLV stream read completes but before the struct is built results in a "use after move" error.Instead, here, we drop the attempt to hide unused variable warnings entirely, dropping the post-TLV-stream access to legacy variables, allowing their use in move semantics for later fields.
For a demonstration of using this, see the last commit at https://git.bitcoin.ninja/?p=rust-lightning;a=log;h=refs/heads/2025-02-3611-ser-impl