Bug 1376446

Summary: segfault when Reapplying slave connection, without changes
Product: [Fedora] Fedora Reporter: Ondrej Lichtner <olichtne>
Component: NetworkManagerAssignee: Beniamino Galvani <bgalvani>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 26CC: bgalvani, dcbw, fgiudici, lkundrak, psimerda, thaller
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
: 1376784 (view as bug list) Environment:
Last Closed: 2017-02-28 10:28:27 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1376784    
Attachments:
Description Flags
Python script reproducing the issue
none
[PATCH] device: fix crash reapplying connection to slave devices none

Description Ondrej Lichtner 2016-09-15 12:47:12 UTC
Created attachment 1201204 [details]
Python script reproducing the issue

Description of problem:
When I try to call Reapply of a bond slave ethernet connection NetworkManager crashes with a segfault (based on log in journalctl). This is using the interface exposed over D-Bus, using python.

Version-Release number of selected component (if applicable):
NetworkManager-1.4.0-4.fc26.x86_64

How reproducible:
always

Steps to Reproduce:
1. add bond connection
2. add slave ethernet connection
3. activate bond connection
4. activate slave connection
5. call Reapply on the device that the slave connection is applied on

Actual results:
Crash with segfault.

Expected results:
Nothing happens. Maybe an error of an invalid or "empty" operation.


Additional info:
python reproducer attached, example use:
./nm_reproducer.py ens9 52:54:00:38:c4:24

Comment 1 Beniamino Galvani 2016-09-16 03:20:13 UTC
Created attachment 1201521 [details]
[PATCH] device: fix crash reapplying connection to slave devices

Comment 2 Thomas Haller 2016-09-16 07:50:37 UTC
(In reply to Beniamino Galvani from comment #1)
> Created attachment 1201521 [details]
> [PATCH] device: fix crash reapplying connection to slave devices

Maybe

	method_old = s_ip4_old ?
		nm_setting_ip_config_get_method (s_ip4_old) :
		NM_SETTING_IP4_CONFIG_METHOD_DISABLED;

should be

	method_old = s_ip4_old ? nm_setting_ip_config_get_method (s_ip4_old) : NULL;

and use nm_streq0() below.

A s_ip4_old==NULL means that it is a slave. A slave doesn't really have ip4.method=DISABLED, it has no IPv4 altogether. Thus, I'd rather compare the NULL there directly, instead of coercing the method to DISABLED.


Of course, in reality it doesn't make a difference, because reapply does not handle the change slave to non-slave or vice versa. Thus, old and new are always either both NULL, or both non-NULL. But still seems more logical to me.

Comment 3 Beniamino Galvani 2016-09-16 08:34:56 UTC
(In reply to Thomas Haller from comment #2)
> A s_ip4_old==NULL means that it is a slave. A slave doesn't really have
> ip4.method=DISABLED, it has no IPv4 altogether. Thus, I'd rather compare the
> NULL there directly, instead of coercing the method to DISABLED.
> 
> 
> Of course, in reality it doesn't make a difference, because reapply does not
> handle the change slave to non-slave or vice versa. Thus, old and new are
> always either both NULL, or both non-NULL. But still seems more logical to
> me.

I tried to make it consistent with function nm_utils_get_ip_config_method() which returns DISABLED/IGNORE for slaves (we can't use that function because it needs a connection).

But as you said, there is really no difference in this case, both ways look fine to me.

Comment 4 Thomas Haller 2016-09-16 12:04:01 UTC
(In reply to Beniamino Galvani from comment #3)
> (In reply to Thomas Haller from comment #2)

ok. lgtm then.

Comment 5 Francesco Giudici 2016-09-16 12:17:26 UTC
lgtm

Comment 7 Fedora End Of Life 2017-02-28 10:18:32 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 26 development cycle.
Changing version to '26'.

Comment 8 Thomas Haller 2017-02-28 10:28:27 UTC
rawhide currently has NM 1.6.0, which has the fix. Closing.