Simple misc cleanups #4227

rustyrussell · 2020-11-25T00:40:32Z

Misc stuff which happened on the way to offers.

niftynei · 2020-11-25T18:01:32Z

common/utils.c

+		return NULL;
+
+	/* Add one for nul term */
+	ret = tal_dup_arr(ctx, char, (const char *)buf, buflen, 1);


should takes be taken into account here?

tal_dup_arr takes already...

What happens if the string is already null-terminated? Could we end up in a situation where we call utf8_str multiple times, each time re-allocating it to append the \0?

Hmm, it would. We should probably reject NULs in strings as not UTF-8.

niftynei

Travis is complaining about memory in the unit tests, otherwise lgtm

ALGRIND=1 valgrind -q --error-exitcode=7 --track-origins=yes --leak-check=full --show-reachable=yes --errors-for-leak-kinds=all common/test/run-bolt11 > /dev/null
==14956== 52 bytes in 1 blocks are still reachable in loss record 1 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FAC7: main (run-bolt11.c:590)
==14956== 
==14956== 52 bytes in 1 blocks are still reachable in loss record 2 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FBD5: main (run-bolt11.c:598)
==14956== 
==14956== 52 bytes in 1 blocks are still reachable in loss record 3 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FD6A: main (run-bolt11.c:610)
==14956== 
==14956== 53 bytes in 1 blocks are still reachable in loss record 4 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FB4E: main (run-bolt11.c:594)
==14956== 
==14956== 53 bytes in 1 blocks are still reachable in loss record 5 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FC5C: main (run-bolt11.c:602)
==14956== 
==14956== 53 bytes in 1 blocks are still reachable in loss record 6 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FDF1: main (run-bolt11.c:614)
==14956== 
==14956== 54 bytes in 1 blocks are still reachable in loss record 7 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FCE3: main (run-bolt11.c:606)
==14956== 
==14956== 54 bytes in 1 blocks are still reachable in loss record 8 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FE78: main (run-bolt11.c:618)
==14956== 
==14956== 54 bytes in 1 blocks are still reachable in loss record 9 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FEFF: main (run-bolt11.c:622)
==14956== 
==14956== 55 bytes in 1 blocks are still reachable in loss record 10 of 12
==14956==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x131CD3: allocate (tal.c:250)
==14956==    by 0x13226C: tal_alloc_ (tal.c:428)
==14956==    by 0x13242D: tal_alloc_arr_ (tal.c:471)
==14956==    by 0x1382DC: decode_d (bolt11.c:185)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FF86: main (run-bolt11.c:626)
==14956== 
==14956== 80 bytes in 1 blocks are still reachable in loss record 11 of 12
==14956==    at 0x4C33D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x12EB63: take_ (take.c:23)
==14956==    by 0x13835C: decode_d (bolt11.c:189)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FF86: main (run-bolt11.c:626)
==14956== 
==14956== 80 bytes in 1 blocks are still reachable in loss record 12 of 12
==14956==    at 0x4C33D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14956==    by 0x12EBF4: take_ (take.c:37)
==14956==    by 0x13835C: decode_d (bolt11.c:189)
==14956==    by 0x139C71: bolt11_decode (bolt11.c:728)
==14956==    by 0x13FF86: main (run-bolt11.c:626)
==14956== 
Makefile:589: recipe for target 'unittest/common/test/run-bolt11' failed
make: *** [unittest/common/test/run-bolt11] Error 7
The command ".travis/build.sh" exited with 2.
cache.2
store build cache

niftynei · 2020-11-25T18:06:13Z

common/utils.c

+
+	for (size_t i = 0; i < buflen; i++) {
+		if (!utf8_decode(&utf8_state, buf[i])) {
+			need_more = true;


is there no early exit for invalid utf-8? the need_more flag flipping is a bit unintuitive

utf8_decode is a bit weird:

/* * Returns false if it needs another character to give results. * Otherwise returns true, @utf8_state can be reused without initializeation, * and sets errno: * 0: success * EINVAL: bad encoding. * EFBIG: not a minimal encoding. * ERANGE: encoding of invalid character. */

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We don't have a problem with them, but callers may; easier to reject bad UTF8 here than let the caller fail when it tries to parse output. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We already do some sanity checks, add this one. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Changed: JSON-RPC: invalid UTF-8 strings now rejected.

There's a 60 second delay in one of the contrib tests, and I just want to run flake8 on my alterations. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Not really a leak, since we eventually process pending, but still: ``` **BROKEN** gossipd: MEMLEAK: 0x5562fa63bab8 **BROKEN** gossipd: label=wire/peer_exp_wiregen.c:3858:u8[] DEBUG gossipd: backtrace: DEBUG gossipd: ccan/ccan/tal/tal.c:442 (tal_alloc_) DEBUG gossipd: ccan/ccan/tal/tal.c:471 (tal_alloc_arr_) DEBUG gossipd: wire/peer_exp_wiregen.c:3858 (fromwire_channel_announcement) DEBUG gossipd: gossipd/routing.c:1706 (handle_channel_announcement) DEBUG gossipd: gossipd/gossipd.c:238 (handle_channel_announcement_msg) DEBUG gossipd: gossipd/gossipd.c:444 (peer_msg_in) DEBUG gossipd: common/daemon_conn.c:31 (handle_read) DEBUG gossipd: ccan/ccan/io/io.c:59 (next_plan) DEBUG gossipd: ccan/ccan/io/io.c:407 (do_plan) DEBUG gossipd: ccan/ccan/io/io.c:417 (io_ready) DEBUG gossipd: ccan/ccan/io/poll.c:445 (io_loop) DEBUG gossipd: gossipd/gossipd.c:1730 (main) **BROKEN** gossipd: parents: **BROKEN** gossipd: gossipd/routing.c:1698:struct pending_cannouncement **BROKEN** gossipd: gossipd/gossipd.c:1700:struct daemon ``` Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

I wanted this for offers, and it's generally useful. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-11-27T03:31:59Z

Thanks, fixed....

cdecker

ACK a211ddb

cdecker · 2020-11-30T16:33:43Z

common/utils.c

+		return NULL;
+
+	/* Add one for nul term */
+	ret = tal_dup_arr(ctx, char, (const char *)buf, buflen, 1);


What happens if the string is already null-terminated? Could we end up in a situation where we call utf8_str multiple times, each time re-allocating it to append the \0?

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-12-01T00:41:04Z

Added rejection of UTF-8 strings with NUL in them (which makes sense, given the UTF-8 API).

rustyrussell added cleanup json-rpc build labels Nov 25, 2020

rustyrussell force-pushed the more-misc-cleanups branch from d060193 to 52b5ee1 Compare November 25, 2020 01:21

niftynei reviewed Nov 25, 2020

View reviewed changes

rustyrussell added 8 commits November 27, 2020 14:00

common/utils: simple utf8 helpers.

9467e0d

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/bolt11: reject bad UTF-8 strings.

1b47efd

We don't have a problem with them, but callers may; easier to reject bad UTF8 here than let the caller fail when it tries to parse output. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

JSON-RPC: don't allow any strings which aren't valid UTF-8.

983cf7f

We already do some sanity checks, add this one. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Changed: JSON-RPC: invalid UTF-8 strings now rejected.

Makefile: separate check-python targets.

135d000

There's a 60 second delay in one of the contrib tests, and I just want to run flake8 on my alterations. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

json: add json_tok_endswith and json_tok_startswith helpers.

1f61bcf

I wanted this for offers, and it's generally useful. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

libplugin: add command_hook_success helper.

e27422d

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/json: json_add_stringn helper.

a211ddb

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the more-misc-cleanups branch from e172b39 to a211ddb Compare November 27, 2020 03:31

rustyrussell mentioned this pull request Nov 29, 2020

Unittest cleanups #4233

Merged

cdecker reviewed Nov 30, 2020

View reviewed changes

ccan: update ccan/utf-8 to reject NULs embedded in strings.

75768b5

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell merged commit ae1a130 into ElementsProject:master Dec 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple misc cleanups #4227

Simple misc cleanups #4227

rustyrussell commented Nov 25, 2020

niftynei Nov 25, 2020

rustyrussell Nov 26, 2020

cdecker Nov 30, 2020

rustyrussell Dec 1, 2020

niftynei left a comment

niftynei Nov 25, 2020

rustyrussell Nov 26, 2020

rustyrussell commented Nov 27, 2020

cdecker left a comment

cdecker Nov 30, 2020

rustyrussell commented Dec 1, 2020

Simple misc cleanups #4227

Simple misc cleanups #4227

Conversation

rustyrussell commented Nov 25, 2020

niftynei Nov 25, 2020

Choose a reason for hiding this comment

rustyrussell Nov 26, 2020

Choose a reason for hiding this comment

cdecker Nov 30, 2020

Choose a reason for hiding this comment

rustyrussell Dec 1, 2020

Choose a reason for hiding this comment

niftynei left a comment

Choose a reason for hiding this comment

niftynei Nov 25, 2020

Choose a reason for hiding this comment

rustyrussell Nov 26, 2020

Choose a reason for hiding this comment

rustyrussell commented Nov 27, 2020

cdecker left a comment

Choose a reason for hiding this comment

cdecker Nov 30, 2020

Choose a reason for hiding this comment

rustyrussell commented Dec 1, 2020