Skip to content

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

Merged
slyon merged 2 commits intocanonical:mainfrom
daniloegea:bond_primary_leak
Apr 15, 2024
Merged

ATTN: parse/bonds: handle same primary in multiple bonds#451
slyon merged 2 commits intocanonical:mainfrom
daniloegea:bond_primary_leak

Conversation

@daniloegea
Copy link
Contributor

@daniloegea daniloegea commented Apr 4, 2024 •
edited by slyon
Loading

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
slyon
slyon approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@slyon slyon left a comment •
edited
Loading

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
Merged
5 tasks
@daniloegea
ATTN: parse/bonds: handle same primary in multiple bonds
11304cf 
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.
@daniloegea
parse/bond: identify bond assigned to primary
656d5d0 
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

daniloegea commented Apr 12, 2024

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
daniloegea added a commit that referenced this pull request Jul 3, 2024
@daniloegea
ATTN: parse/bonds: handle same primary in multiple bonds (#451)
af342a2 
* 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

Reviewers

@slyon slyon Awaiting requested review from slyon

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@daniloegea @slyon