Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ATTN: parse/bonds: handle same primary in multiple bonds #451

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

daniloegea
Copy link
Contributor

@daniloegea daniloegea commented Apr 4, 2024

Error out if an interface is declared as primary of multiple bonds. Besides being wrong it's causing a memory leak.

Description

Found with https://github.com/daniloegea/netplan-fuzz

Test case:

network:
  version: 2
  ethernets:
    eno1: {}
  bonds:
    bond0:
      parameters:
        primary: eno1
    bond1:
      parameters:
        primary: eno1

Result

$ ./build/src/generate --root-dir /tmp/fakeroot

=================================================================
==1732948==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 5 byte(s) in 1 object(s) allocated from:
    #0 0x711093a8bb27 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x711093794ed9 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62ed9) (BuildId: ab12e476c12cb3b1fbdd6b86b800846e3dd61000)
    #2 0x7110937aa8a8 in g_strdup (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x788a8) (BuildId: ab12e476c12cb3b1fbdd6b86b800846e3dd61000)
    #3 0x7110939154b9 in g_strdup_inline /usr/include/glib-2.0/glib/gstrfuncs.h:321
    #4 0x7110939154b9 in handle_bond_primary_member ../src/parse.c:2316
    #5 0x711093906547 in process_mapping ../src/parse.c:316
    #6 0x711093915fef in handle_bonding ../src/parse.c:2417
    #7 0x711093906336 in process_mapping ../src/parse.c:310
    #8 0x71109391b5b7 in handle_network_type ../src/parse.c:3302
    #9 0x711093906336 in process_mapping ../src/parse.c:310
    #10 0x7110939063bf in process_mapping ../src/parse.c:312
    #11 0x71109391c4c0 in process_document ../src/parse.c:3446
    #12 0x71109391cb46 in _netplan_parser_load_single_file ../src/parse.c:3504
    #13 0x71109391cebc in netplan_parser_load_yaml ../src/parse.c:3543
    #14 0x7110939356a0 in netplan_parser_load_yaml_hierarchy ../src/util.c:661
    #15 0x5a1f00e1f5ec in main ../src/generate.c:252
    #16 0x7110933131c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #17 0x71109331328a in __libc_start_main_impl ../csu/libc-start.c:360
    #18 0x5a1f00e1d984 in _start (/workspace/netplan-daniloegea/build/src/generate+0x4984) (BuildId: f5aec88fce01a307e0e73c220482bbfa56019c2f)

SUMMARY: AddressSanitizer: 5 byte(s) leaked in 1 allocation(s).

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@daniloegea daniloegea marked this pull request as ready for review April 4, 2024 14:13
@daniloegea daniloegea requested a review from slyon April 4, 2024 14:14
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall!

This makes parsing stricter, so we need to be very careful with backports/SRUs. Please prefix the commit message with "ATTN: ..." so that this is more obvious for future backporting work.

@slyon slyon changed the title parse/bonds: handle same primary in multiple bonds ATTN: parse/bonds: handle same primary in multiple bonds Apr 10, 2024
@slyon slyon mentioned this pull request Apr 10, 2024
5 tasks
Error out if an interface is declared as primary of multiple bonds.
Besides being wrong it's causing a memory leak.

ATTN: as this makes the parser stricter, it probably should not be
included in backports/SRUs. Consider converting the error to a warning
and freeing the memory allocation to avoid the leak.
Look for the bond that already has the primary member assigned and
identify it in the error message.
@daniloegea daniloegea requested a review from slyon April 12, 2024 10:34
@daniloegea
Copy link
Contributor Author

Thanks for the suggestions, Lukas. I added a new commit with my changes to make it easier to review. But they should probably be squashed for merge.

@slyon slyon merged commit 7e22e6f into canonical:main Apr 15, 2024
14 of 16 checks passed
daniloegea added a commit that referenced this pull request Jul 3, 2024
* ATTN: parse/bonds: handle same primary in multiple bonds

Error out if an interface is declared as primary of multiple bonds.
Besides being wrong it's causing a memory leak.

ATTN: as this makes the parser stricter, it probably should not be
included in backports/SRUs. Consider converting the error to a warning
and freeing the memory allocation to avoid the leak.

* parse/bond: identify bond assigned to primary

Look for the bond that already has the primary member assigned and
identify it in the error message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants