Compiler error #9
Open
Conversation
This file contains 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
Noticed by stracing for an unrelated problem. Signed-off-by: Rusty Russell <[email protected]>
1. Freeing an unconfirmed channel already releases the subd, so don't do that explicitly. 2. Use channel->owner to transfer ownership where possible, using channel_set_owner() which handles all the cases. This simplifies the code and makes it more readable, IMHO. Signed-off-by: Rusty Russell <[email protected]>
We don't need the other "gossip" messages in our echo-suppression filter. Signed-off-by: Rusty Russell <[email protected]>
They all returned the same thing anyway. Signed-off-by: Rusty Russell <[email protected]>
connectd is going to end up using this do demux; make it fast and complete. Fixing this reveals a problem in openingd: it now extracts the channel_id from funding_signed (which is where we transition off the temporary), and gets upset. So fix that. Signed-off-by: Rusty Russell <[email protected]>
The "read until closed" trick doesn't work if the other end doesn't close (as found in the next patch, where we use DEV_DISCONNECT_DISABLE_AFTER). Signed-off-by: Rusty Russell <[email protected]>
These would have to be done by connectd, not the local daemon, once it's intermediating. Otherwise the remote peer won't see any change. Signed-off-by: Rusty Russell <[email protected]>
Once connectd is doing this, we can't close as soon as we send, and in fact we can't do 'fail write' either. Signed-off-by: Rusty Russell <[email protected]>
…ommit It was always a hack, but an impossible one once connectd will be interpreting dev-disconnect! Signed-off-by: Rusty Russell <[email protected]>
Increasingly we want to know is it local, and get the direction: it's more efficient to do both at once. Signed-off-by: Rusty Russell <[email protected]>
local_chan was mainly around so we could "soft" disable channels (and really disable them once we used the channel_update in an error message). Instead we introduce the idea of a "deferred_update": it's either deferred indefinitely (a peer goes offline, if we need to send it in an error we'll apply it immediatly), or simply delayed to avoid spamming everyone. The resulting rewrite is much clearer, IMHO. Signed-off-by: Rusty Russell <[email protected]>
They all returned the next io_plan, but it was always the same. Signed-off-by: Rusty Russell <[email protected]>
We want to have a real (persistent) struct peer eventually. Signed-off-by: Rusty Russell <[email protected]>
connectd will be keeping the conn open, so it needs to free this "conn_timeout" timer. Pass it through, so we can do that. Signed-off-by: Rusty Russell <[email protected]>
Instead of passing the incoming socket to lightningd for the subdaemon, create a new one and simply shuffle data between them, keeping connectd in the loop. For the moment, we don't decrypt at all, just shuffle. This means our buffer code is kind of a hack, but that goes away once we start actually decrypting and understanding message boundaries. Signed-off-by: Rusty Russell <[email protected]>
This avoids changes to crypto_sync which are coming in next patch. Signed-off-by: Rusty Russell <[email protected]>
We temporarily hack to sync_crypto_write/sync_crypto_read functions to not do any crypto, and do it all in connectd. Signed-off-by: Rusty Russell <[email protected]>
Now connectd is doing the crypto, we can use normal wire io. We create helper functions to clearly differentiate between "peer" comms and intra-daemon comms though. Signed-off-by: Rusty Russell <[email protected]>
Now that connectd does the crypto, no need to hand around crypto_state. Signed-off-by: Rusty Russell <[email protected]>
As connectd handles more packets itself, or diverts them to/from gossipd, it's the only place we can implement the dev_disconnect logic. Signed-off-by: Rusty Russell <[email protected]>
channeld can't do it any more: it's using local sockets. Connectd can do it, and simply does it by type. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
We actually intercept the gossip_timestamp_filter, so the gossip_store mechanism inside the per-peer daemon never kicks off for normal connections. The gossipwith tool doesn't set OPT_GOSSIP_QUERIES, so it gets both, but that only effects one place. Signed-off-by: Rusty Russell <[email protected]>
We now let gossipd do it. This also means there's nothing left in 'struct per_peer_state' to send across the wire (the fds are sent separately), so that gets removed from wire messages too. Signed-off-by: Rusty Russell <[email protected]>
Just hand around the two fds. We have to take a little care to close them manually now, though. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
We're the only ones who use it now, so put our fields inside it and make it local. Signed-off-by: Rusty Russell <[email protected]>
It's weird to have connectd ask gossipd, when lightningd can just do it and hand all the addresses together. Signed-off-by: Rusty Russell <[email protected]>
This is in preparation for gossipd feeding us the latest channel_updates, rather than having lightningd and channeld query gossipd when it wants to send an onion error with an update included. This means gossipd will start telling us the updates, so we need the channels loaded first. Signed-off-by: Rusty Russell <[email protected]>
We want it to keep the latest, so it can make its own error msgs without asking us. This installs (but does not use!) the message handler. Signed-off-by: Rusty Russell <[email protected]>
Now we only send and receive gossip messages on this fd. Signed-off-by: Rusty Russell <[email protected]>
…msg. We don't need the connection to ourselves! Signed-off-by: Rusty Russell <[email protected]>
I'm guessing doing a 16 bit swap on WIRE_GOSSIP_STORE_ENDED is the solution
Error on Mac OSX 11.6:
common/gossip_store.c:132:16: error: result of comparison of constant 152043520 with expression of type 'be16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (buf.type == CPU_TO_BE32(WIRE_GOSSIP_STORE_ENDED))
$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 13.0.0 (clang-1300.0.29.30)
Target: x86_64-apple-darwin20.6.0
rustyrussell
force-pushed
the
connectd-demux-part-2
branch
from
8950384
to
0d7bf21
Compare
rustyrussell
force-pushed
the
connectd-demux-part-2
branch
17 times, most recently
from
fa513c9
to
58c9044
Compare
rustyrussell
force-pushed
the
connectd-demux-part-2
branch
3 times, most recently
from
1c4481b
to
93f8a11
Compare
rustyrussell
pushed a commit
that referenced
this pull request
Mar 1, 2022
All build flags and (experimental) options make it hard to find out what features are supported or enabled. And the undocumented `--list-features-only`, does not account for all our featurebits, for example bit 55 (keysend). Changelog-Added: JSON-RPC: `getinfo` result now includes `our_features` (bits) for various Bolt #9 contexts
rustyrussell
pushed a commit
that referenced
this pull request
Feb 14, 2023
This will fix a crash that I caused on armv7 and by looking inside the coredump with gdb (by adding an assert on n that must be different from null) I get the following stacktrace ``` (gdb) bt \#0 0x00000000 in ?? () \#1 0x0043a038 in send_backtrace (why=0xbe9e3600 "FATAL SIGNAL 11") at common/daemon.c:36 \#2 0x0043a0ec in crashdump (sig=11) at common/daemon.c:46 \#3 <signal handler called> \#4 0x00406d04 in node_announcement (map=0x938ecc, nann_off=495146) at common/gossmap.c:586 \#5 0x00406fec in map_catchup (map=0x938ecc, num_rejected=0xbe9e3a40) at common/gossmap.c:643 \#6 0x004073a4 in load_gossip_store (map=0x938ecc, num_rejected=0xbe9e3a40) at common/gossmap.c:697 \#7 0x00408244 in gossmap_load (ctx=0x0, filename=0x4e16b8 "gossip_store", num_channel_updates_rejected=0xbe9e3a40) at common/gossmap.c:976 \#8 0x0041a548 in init (p=0x93831c, buf=0x9399d4 "\n\n{\"jsonrpc\":\"2.0\",\"id\":\"cln:init#25\",\"method\":\"init\",\"params\":{\"options\":{},\"configuration\":{\"lightning-dir\":\"/home/vincent/.lightning/testnet\",\"rpc-file\":\"lightning-rpc\",\"startup\":true,\"network\":\"te"..., config=0x939cdc) at plugins/topology.c:622 \#9 0x0041e5d0 in handle_init (cmd=0x938934, buf=0x9399d4 "\n\n{\"jsonrpc\":\"2.0\",\"id\":\"cln:init#25\",\"method\":\"init\",\"params\":{\"options\":{},\"configuration\":{\"lightning-dir\":\"/home/vincent/.lightning/testnet\",\"rpc-file\":\"lightning-rpc\",\"startup\":true,\"network\":\"te"..., params=0x939c8c) at plugins/libplugin.c:1208 \#10 0x0041fc04 in ld_command_handle (plugin=0x93831c, toks=0x939bec) at plugins/libplugin.c:1572 \#11 0x00420050 in ld_read_json_one (plugin=0x93831c) at plugins/libplugin.c:1667 \#12 0x004201bc in ld_read_json (conn=0x9391c4, plugin=0x93831c) at plugins/libplugin.c:1687 \#13 0x004cb82c in next_plan (conn=0x9391c4, plan=0x9391d8) at ccan/ccan/io/io.c:59 \#14 0x004cc67c in do_plan (conn=0x9391c4, plan=0x9391d8, idle_on_epipe=false) at ccan/ccan/io/io.c:407 \ElementsProject#15 0x004cc6dc in io_ready (conn=0x9391c4, pollflags=1) at ccan/ccan/io/io.c:417 \ElementsProject#16 0x004cf8cc in io_loop (timers=0x9383c4, expired=0xbe9e3ce4) at ccan/ccan/io/poll.c:453 \ElementsProject#17 0x00420af4 in plugin_main (argv=0xbe9e3eb4, init=0x41a46c <init>, restartability=PLUGIN_STATIC, init_rpc=true, features=0x0, commands=0x6167e8 <commands>, num_commands=4, notif_subs=0x0, num_notif_subs=0, hook_subs=0x0, num_hook_subs=0, notif_topics=0x0, num_notif_topics=0) at plugins/libplugin.c:1891 \ElementsProject#18 0x0041a6f8 in main (argc=1, argv=0xbe9e3eb4) at plugins/topology.c:679 ``` I do not know if this is a solution because I do not know when I can parse a node announcement for a node that it is not longer in the gossip map. So, I hope this is just usefult for @rustyrussell Changelog-Fixed: fixes `FATAL SIGNAL 11` on gossmap node announcement parsing. Signed-off-by: Vincenzo Palazzo <[email protected]>
rustyrussell
pushed a commit
that referenced
this pull request
Mar 23, 2023
The issue is that common_setup() wasn't called by the fuzz target,
leaving secp256k1_ctx as NULL.
UBSan error:
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" \
./fuzz-channel_id crash-1575b41ef09e62e4c09c165e6dc037a110b113f2
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1153355603
INFO: Loaded 1 modules (25915 inline 8-bit counters): 25915 [0x563bae7ac3a8, 0x563bae7b28e3),
INFO: Loaded 1 PC tables (25915 PCs): 25915 [0x563bae7b28e8,0x563bae817c98),
./fuzz-channel_id: Running 1 inputs 1 time(s) each.
Running: crash-1575b41ef09e62e4c09c165e6dc037a110b113f2
bitcoin/pubkey.c:22:33: runtime error: null pointer passed as argument 1, which is declared to never be null
external/libwally-core/src/secp256k1/include/secp256k1.h:373:3: note: nonnull attribute specified here
#0 0x563bae41e3db in pubkey_from_der bitcoin/pubkey.c:19:7
#1 0x563bae4205e0 in fromwire_pubkey bitcoin/pubkey.c:111:7
#2 0x563bae46437c in run tests/fuzz/fuzz-channel_id.c:42:3
#3 0x563bae2f6016 in LLVMFuzzerTestOneInput tests/fuzz/libfuzz.c:23:2
#4 0x563bae20a450 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)
#5 0x563bae1f4c3f in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long)
#6 0x563bae1fa6e6 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))
#7 0x563bae223052 in main (tests/fuzz/fuzz-channel_id+0x181052) (BuildId: f7f56e14ffc06df54ab732d79ea922e773de1f25)
#8 0x7fa7fa113082 in __libc_start_main
#9 0x563bae1efbdd in _start
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior bitcoin/pubkey.c:22:33 in
rustyrussell
pushed a commit
that referenced
this pull request
Apr 5, 2023
pubkey_from_hexstr() was failing, which we didn't notice because we
weren't checking the return value. The problem was that we were passing
it a strlen that was half the actual length.
Relevant error:
[libsecp256k1] illegal argument: !secp256k1_fe_is_zero(&ge->x)
==417723== ERROR: libFuzzer: deadly signal
#7 0x7f5deaacc7fb in abort
#8 0x51b0b0 in secp256k1_default_illegal_callback_fn secp256k1.c
#9 0x51bd8e in secp256k1_ec_pubkey_serialize
#10 0x4e235b in pubkey_to_der bitcoin/pubkey.c:29:7
#11 0x4e2941 in pubkey_cmp bitcoin/pubkey.c:89:2
#12 0x4e333d in bitcoin_redeem_2of2 bitcoin/script.c:144:6
#13 0x4f1396 in run tests/fuzz/fuzz-close_tx.c:78:19
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.
@rustyrussell
I'm guessing doing a 16 bit swap on WIRE_GOSSIP_STORE_ENDED is the solution
Error on Mac OSX 11.6:
common/gossip_store.c:132:16: error: result of comparison of constant 152043520 with expression of type 'be16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (buf.type == CPU_TO_BE32(WIRE_GOSSIP_STORE_ENDED))
$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 13.0.0 (clang-1300.0.29.30)
Target: x86_64-apple-darwin20.6.0