network: sync link state file on dbus call, and ndisc cleanups by yuwata · Pull Request #16532 · systemd/systemd

yuwata · 2020-07-21T07:17:35Z

No description provided.

mrc0mmand · 2020-07-21T08:26:36Z

==125445==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7f4daa7346a8 at pc 0x7f4dafd1471c bp 0x7ffc03b186f0 sp 0x7ffc03b17ea0
READ of size 16 at 0x7f4daa7346a8 thread T0
    #0 0x7f4dafd1471b in __asan_memcpy (/usr/lib/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xec71b)
    #1 0x5624c4fa3313 in address_get /systemd-meson-build/../build/src/network/networkd-address.c:413:28
    #2 0x5624c5071808 in ndisc_router_process_autonomous_prefix /systemd-meson-build/../build/src/network/networkd-ndisc.c:372:21
    #3 0x5624c506ef59 in ndisc_router_process_options /systemd-meson-build/../build/src/network/networkd-ndisc.c:707:37
    #4 0x5624c506c40a in ndisc_router_handler /systemd-meson-build/../build/src/network/networkd-ndisc.c:770:13
    #5 0x5624c50690b1 in ndisc_handler /systemd-meson-build/../build/src/network/networkd-ndisc.c:792:21
    #6 0x5624c5117ec7 in ndisc_callback /systemd-meson-build/../build/src/libsystemd-network/sd-ndisc.c:43:9
    #7 0x5624c5117a06 in ndisc_handle_datagram /systemd-meson-build/../build/src/libsystemd-network/sd-ndisc.c:201:9
    #8 0x5624c5116520 in ndisc_recv /systemd-meson-build/../build/src/libsystemd-network/sd-ndisc.c:254:16
    #9 0x7f4daf6f176f in source_dispatch /systemd-meson-build/../build/src/libsystemd/sd-event/sd-event.c:3190:21
    #10 0x7f4daf6f0749 in sd_event_dispatch /systemd-meson-build/../build/src/libsystemd/sd-event/sd-event.c:3631:21
    #11 0x7f4daf6f317c in sd_event_run /systemd-meson-build/../build/src/libsystemd/sd-event/sd-event.c:3689:21
    #12 0x7f4daf6f3bdd in sd_event_loop /systemd-meson-build/../build/src/libsystemd/sd-event/sd-event.c:3711:21
    #13 0x5624c4ebfabc in run /systemd-meson-build/../build/src/network/networkd.c:123:13
    #14 0x5624c4ebf264 in main /systemd-meson-build/../build/src/network/networkd.c:130:1
    #15 0x7f4daebcf001 in __libc_start_main (/usr/lib/libc.so.6+0x27001)
    #16 0x5624c4ebf17d in _start (/systemd-meson-build/systemd-networkd+0x22d17d)
Address 0x7f4daa7346a8 is located in stack of thread T0 at offset 168 in frame
    #0 0x5624c50712cf in ndisc_router_process_autonomous_prefix /systemd-meson-build/../build/src/network/networkd-ndisc.c:318
  This frame has 10 object(s):
    [32, 36) 'lifetime_valid' (line 319)
    [48, 52) 'lifetime_preferred' (line 319)
    [64, 72) 'addresses' (line 320)
    [96, 104) 'address' (line 321)
    [128, 144) 'addr' (line 322)
    [160, 168) 'a' (line 322) <== Memory access at offset 168 overflows this variable
    [192, 196) 'prefixlen' (line 323)
    [208, 216) 'time_now' (line 324)
    [240, 256) 'i' (line 325)
    [272, 280) 'existing_address' (line 369)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/usr/lib/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xec71b) in __asan_memcpy

yuwata · 2020-07-21T09:09:10Z

@mrc0mmand Thanks! I hope now it is fixed.

yuwata · 2020-07-21T12:26:43Z

@poettering Thank you for your review. I've addressed all your comments. PTAL.

keszybz

Hmm, I'm a bit confused here. I would expect a memory leak to happen in the last test, but the CI passes. Either the CI does not test this particular code path under valgrind, or my understanding of the code is wrong.

keszybz · 2020-07-22T10:25:11Z

set_put returns 0 when the item is already in the set, and 1 when it is not. So the check for -EEXIST is not doing the right thing.

If I understand correctly...

set_put returns 0 when the item is already in the set, and 1 when it is not.

Right.

So the check for -EEXIST is not doing the right thing.

~~But set_put() returns -EEXIST if the set contains an element whose hash value is equivalent to the new one. So, I think in this case checking -EEXIST is correct. No?~~

From the log in systemd-networkd-tests.py NetworkdRATests.test_ipv6_token_static_multiple

systemd-networkd[862770]: /run/systemd/network/ipv6-prefix-veth-token-static-multiple.network:7: IPv6 token '::1a:2b:3c:4d' is duplicated, ignoring: File exists systemd-networkd[862770]: /run/systemd/network/ipv6-prefix-veth-token-static-multiple.network:8: IPv6 token '::1a:2b:3c:4d' is duplicated, ignoring: File exists

So, the check -EEXIST works correctly.

Ah... ordered_set_put() is a wrapper of hashmap_put(). But set_put() is not... Ugh.

So, you are right here, as here we use set_put().

keszybz · 2020-07-22T10:27:50Z

But here, the code is correct, as this is ordered_set_put().

Hmmm, see test-ordered-set.c, test_set_put(). The return value is 0.

Please see #16561.

Right, so we get -EEXIST because ipv6_token_hash_ops != trivial_hash_ops`. I think we should fix this at some point, but not in this PR.

This also changes the stored type from int to uint8_t in order to make hash value endianness independent.

…_state_file

The Address objects in the set generated by ndisc_router_generate_addresses() have the equivalent prefixlen, flags, prefered lifetime. This commit makes ndisc_router_generate_addresses() return Set of in6_addr.

keszybz

LGTM. There's some things to potentially change in the future, but the code is OK as is.

keszybz · 2020-07-23T14:28:55Z

+        if (!in)
+                return;
+
+        siphash24_compress(in, strlen(in), state);


I'm wondering if this should be strlen() + 1 so that the result for NULL and "" is different.

yuwata added the network label Jul 21, 2020

yuwata force-pushed the network-sync-state-file branch from 9f99864 to 8286351 Compare July 21, 2020 09:08

poettering requested changes Jul 21, 2020

View reviewed changes

Comment thread src/basic/siphash24.h Outdated

Comment thread src/basic/siphash24.h Outdated

Comment thread src/basic/hash-funcs.c Outdated

Comment thread src/basic/in-addr-util.c Outdated

Comment thread src/network/networkd-ndisc.c Outdated

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jul 21, 2020

yuwata force-pushed the network-sync-state-file branch from 8286351 to fe10ed8 Compare July 21, 2020 12:22

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jul 21, 2020

keszybz reviewed Jul 22, 2020

View reviewed changes

yuwata added 7 commits July 22, 2020 19:55

util: make siphash24_compress_boolean() inline

6c04fcc

This also changes the stored type from int to uint8_t in order to make hash value endianness independent.

util: introduce siphash24_compress_string()

1c568d6

tree-wide: use siphash24_compress_string() where it is applicable

f281fc1

network: introduce link_save_and_clean()

c2a6595

network: make bus methods sync link state file

1b14222

test-network: drop unnecessary sleep() in NetworkdStateFileTests.test…

f91b234

…_state_file

util: use IN6_ARE_ADDR_EQUAL() macro

3dbd8a1

yuwata force-pushed the network-sync-state-file branch from fe10ed8 to 5984396 Compare July 22, 2020 10:55

yuwata added 3 commits July 22, 2020 20:26

network: ndisc: do not store duplicated data in Set

92ee90a

The Address objects in the set generated by ndisc_router_generate_addresses() have the equivalent prefixlen, flags, prefered lifetime. This commit makes ndisc_router_generate_addresses() return Set of in6_addr.

network: ndisc: ignore duplicated IPv6Token=

2c62149

test-network: add test for duplicated IPv6Token=

7f8c1e9

yuwata force-pushed the network-sync-state-file branch from 5984396 to 7f8c1e9 Compare July 22, 2020 11:26

keszybz approved these changes Jul 23, 2020

View reviewed changes

keszybz merged commit 01b9294 into systemd:master Jul 23, 2020

yuwata deleted the network-sync-state-file branch July 23, 2020 14:55

Conversation

yuwata commented Jul 21, 2020

Uh oh!

mrc0mmand commented Jul 21, 2020

Uh oh!

yuwata commented Jul 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuwata commented Jul 21, 2020

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

yuwata Jul 22, 2020 •

edited

Loading