pay crashes on json_escape_needed on unknown odd channel WIRE_UNKNOWN_NEXT_PEER

[fiatjaf]

Issue and Steps to Reproduce

A random payment just caused pay to crash:

2020-04-01T16:53:47.007Z INFO 033868c219bdb51a33560d854d500fe7d3898a1ad9e05dd89d0007e11313588500-chan#4900: htlc 235 failed from 2th node with code 0x400a (WIRE_UNKNOWN_NEXT_PEER)
pay: ccan/ccan/json_out/json_out.c:254: json_out_addv: Assertion `quote || !json_escape_needed(dst, fmtlen)' failed.
pay: FATAL SIGNAL 6 (version v0.8.0-371-g8c984fb-modded)
2020-04-01T16:53:47.017Z UNUSUAL gossipd: routing_failure: Channel 16718782x2081716x44060 unknown
0x5648c2398137 send_backtrace
        common/daemon.c:39
0x5648c23981dd crashdump
        common/daemon.c:52
0x7fc00b890f1f ???
        ???:0
0x7fc00b890e97 ???
        ???:0
0x7fc00b892800 ???
        ???:0
0x7fc00b882399 ???
        ???:0
0x7fc00b882411 ???
        ???:0
0x5648c23b22c8 json_out_addv
        ccan/ccan/json_out/json_out.c:254
0x5648c239be71 json_add_member
        common/json_stream.c:160
0x5648c2384c83 waitsendpay_expired
        plugins/pay.c:265
0x5648c238557d waitsendpay_error
        plugins/pay.c:515
0x5648c238abfd handle_rpc_reply
        plugins/libplugin.c:534
0x5648c238b260 rpc_read_response_one
        plugins/libplugin.c:645
0x5648c238b351 rpc_conn_read_response
        plugins/libplugin.c:664
0x5648c23abe03 next_plan
        ccan/ccan/io/io.c:59
0x5648c23ac980 do_plan
        ccan/ccan/io/io.c:407
0x5648c23ac9be io_ready
        ccan/ccan/io/io.c:417
0x5648c23aeb84 io_loop
        ccan/ccan/io/poll.c:445
0x5648c238d135 plugin_main
        plugins/libplugin.c:1194
0x5648c238985e main
        plugins/pay.c:1709
0x7fc00b873b96 ???
        ???:0
0x5648c2383f49 ???
        ???:0
0xffffffffffffffff ???
        ???:0
pay: FATAL SIGNAL 11 (version v0.8.0-371-g8c984fb-modded)
0x5648c2398137 send_backtrace
        common/daemon.c:39
0x5648c23981dd crashdump
        common/daemon.c:52
0x7fc00b890f1f ???
        ???:0
0x0 ???
        ???:0

getinfo output

{
   "id": "02c16cca44562b590dd279c942200bdccfd4f990c3a69fad620c10ef2f8228eaff",
   "alias": "@lntxbot",
   "color": "296683",
   "num_peers": 40,
   "num_pending_channels": 1,
   "num_active_channels": 39,
   "num_inactive_channels": 0,
   "address": [
      {
         "type": "ipv6",
         "address": "2a01:4f8:c0c:7b31::1",
         "port": 9735
      }
   ],
   "binding": [
      {
         "type": "ipv6",
         "address": "::",
         "port": 9735
      }
   ],
   "version": "v0.8.0-371-g8c984fb",
   "blockheight": 623949,
   "network": "bitcoin",
   "msatoshi_fees_collected": 872240,
   "fees_collected_msat": "872240msat",
   "lightning-dir": "/home/fiatjaf/.lightning/bitcoin"
}

[ZmnSCPxj]

The issue seems to be a misuse of json_add_member. This function is only used for primitive types (numbers, true, false, null) with quote == false, or for strings if quote == true.

Since the route is already in JSON, we should use some kind of direct-add function instead. Lemme dig through the codebase a little...

[ZmnSCPxj]

The line below uses json_out_add_raw on a member of struct json_stream, which "shouldn't" be done because it is an abstraction violation, but also implies that common/json_stream.h does not provide a complete interface
.

lightning/plugins/pay.c

Line 835 in 158d221

json_out_add_raw(req->js->jout, "route", attempt->route);

This seems to be the "correct" way to print the route.

This line uses json_add_member using quote == true aka string mode:

lightning/plugins/pay.c

Lines 1419 to 1420 in 158d221

	if (attempt->route)
	json_add_member(ret, "route", true, "%s", attempt->route);

The first is probably the correct way to add route to JSON streams, and the second should probably be modified.

I think we should add a json_add_jsonstr function to common/json_stream.h which just calls json_out_add_raw underneath (possibly a formatted function?) without exposing json_out, because abstraction violation.

(Other uses of json_out in code that normally uses json_stream should themselves probably have abstraction-wrappers of calls to json_out code, eg.)

lightning/plugins/pay.c

Lines 267 to 268 in 158d221

	json_out_add_splice(data->jout, "failure",
	pc->ps->attempts[i].failure);

lightning/plugins/pay.c

Lines 1422 to 1423 in 158d221

	if (attempt->failure)
	json_out_add_splice(ret->jout, "failure", attempt->failure);

[ZmnSCPxj]

Now we just need to design a test for this. Probably start two nodes, the payee node having an htlc_accepted-hooked plugin which delays a payment for say 3 seconds, then invoke pay on the payer node with a retry_for timeout of 1second, which should tickle this specific case. Though we might need more complex test depending on how route gets generated.

[niftynei]

I went ahead and just switched the 'quoted' from true to false, but happy to swap this out for a more robust fix.