After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 797022 - NULL pointer deref in add_ip_config (src/dns/nm-dns-dnsmasq.c:186) because ip_data->domains.reverse is NULL
NULL pointer deref in add_ip_config (src/dns/nm-dns-dnsmasq.c:186) because ip...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: IP and DNS config
1.12.x
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2018-08-24 12:10 UTC by Michael Schaller
Modified: 2018-09-14 08:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] dns: dnsmasq: avoid crash when no reverse domains exist (1.27 KB, patch)
2018-09-13 12:56 UTC, Beniamino Galvani
none Details | Review
[PATCH v2] dns: dnsmasq: avoid crash when no reverse domains exist (1.32 KB, patch)
2018-09-13 12:59 UTC, Beniamino Galvani
none Details | Review

Description Michael Schaller 2018-08-24 12:10:33 UTC
Backtrace:

  • #0 add_ip_config
    at src/dns/nm-dns-dnsmasq.c line 186
  • #1 update
    at src/dns/nm-dns-dnsmasq.c line 396
  • #2 update_dns
    at src/dns/nm-dns-manager.c line 1409
  • #3 nm_dns_manager_end_updates
    at src/dns/nm-dns-manager.c line 1737
  • #4 ffi_call_unix64
  • #5 ffi_call
  • #6 g_cclosure_marshal_generic_va
    at ../../../../gobject/gclosure.c line 1604
  • #7 _g_closure_invoke_va
    at ../../../../gobject/gclosure.c line 867
  • #8 g_signal_emit_valist
    at ../../../../gobject/gsignal.c line 3300
  • #9 g_signal_emit
    at ../../../../gobject/gsignal.c line 3447
  • #10 nm_device_set_ip_config
    at src/devices/nm-device.c line 11456
  • #11 _cleanup_generic_post
    at src/devices/nm-device.c line 13524
  • #12 nm_device_cleanup
    at src/devices/nm-device.c line 13684
  • #13 _set_state_full
    at src/devices/nm-device.c line 14050
  • #14 nm_device_state_changed
    at src/devices/nm-device.c line 14306
  • #15 _set_unmanaged_flags
    at src/devices/nm-device.c line 12739
  • #16 nm_device_set_unmanaged_by_flags
    at src/devices/nm-device.c line 12780
  • #17 remove_device
    at src/nm-manager.c line 1630
  • #18 _platform_link_cb_idle
    at src/nm-manager.c line 3180
  • #19 g_main_dispatch
    at ../../../../glib/gmain.c line 3148
  • #20 g_main_context_dispatch
    at ../../../../glib/gmain.c line 3813
  • #21 g_main_context_iterate
    at ../../../../glib/gmain.c line 3886
  • #22 g_main_loop_run
    at ../../../../glib/gmain.c line 4082
  • #23 main
    at src/main.c line 438


The code in question:

                for (j = 0; ip_data->domains.reverse[j]; j++) {
                        add_dnsmasq_nameserver (self, servers,
                                                ip_addr_to_string_buf,
                                                ip_data->domains.reverse[j]);
                }



The code expects 'reverse' to never be NULL but it was:

(gdb) print ip_data
$2 = (const NMDnsIPConfigData *) 0x7f9804013590

(gdb) print ip_data->domains
$3 = {search = 0x560fe4e51120, reverse = 0x0}



I sadly don't know how this can happen or under which cirumstances this happens.
Comment 1 Beniamino Galvani 2018-08-27 16:22:59 UTC
Can you reliably reproduce the problem? The obvious fix for this is:

@@ -180,16 +180,18 @@ add_ip_config (NMDnsDnsmasq *self, GVariantBuilder *servers, const NMDnsIPConfig
                        add_dnsmasq_nameserver (self,
                                                servers,
                                                ip_addr_to_string_buf,
                                                domain[0] ? domain : NULL);
                }

-               for (j = 0; ip_data->domains.reverse[j]; j++) {
-                       add_dnsmasq_nameserver (self, servers,
-                                               ip_addr_to_string_buf,
-                                               ip_data->domains.reverse[j]);
+               if (ip_data->domains.reverse) {
+                       for (j = 0; ip_data->domains.reverse[j]; j++) {
+                               add_dnsmasq_nameserver (self, servers,
+                                                       ip_addr_to_string_buf,
+                                                       ip_data->domains.reverse[j]);
+                       }
                }
        }
 }

but I wonder how reverse domains can be empty (i.e. the device has no IP address) when there is a nameserver.  Maybe there is another bug elsewhere...
Comment 2 Michael Schaller 2018-08-28 06:35:20 UTC
I sadly can't reproduce the issue but I'll ask my colleague who had this segfault to directly comment on this bug. Maybe he has more details.

I was also wondering how this can happen and if this is something that shouldn't happen. Hence I didn't include a patch. ;-)
Comment 3 Markus Teich 2018-08-28 12:50:34 UTC
I can reproduce, but I don't have instructions for how to repro. I see from syslog that it only seems to crash with my home network and it seems to crash whenever I disconnect from my home network. Unfortunately I am unable to test/reproduce until ~mid September.

My home network has two APs.
- AP1 is a Fritz! Box connecting to my uplink and serves as DHCP server with static IP per MAC. Iirc, AP1 also has a cashing DNS server and adds the typical "fritz.box" and a few other names.
- AP2 is some (tp-link?) AP, which serves as the wifi hotspot and also passes through ethernet connections to a 1G ethernet switch. AP2 is running OpenWRT.

My laptop is connected via wifi directly with AP2. It is also connected to a thunderbolt3 docking station which has a 1G connection to AP2 through the ethernet switch.

Most of the time, my laptop is connected both via wifi and ethernet and when unplugging from the dock (which seems to be correlated with the segfault), only the wired connection is disconnected.

My network setup did not change since I upgraded to NM v1.12.0-5 and I did not see these crashes before when I was running NM v1.10.2-1, so I'm pretty sure this bug was introduced between these two versions.

I can add my network.log from shortly before a crash:

Aug 23 20:50:24 $HOSTNAME wpa_supplicant[1290]: nl80211: Ignore RTM_NEWLINK event for foreign ifindex 5
Aug 23 20:50:24 $HOSTNAME dhclient[89771]: receive_packet failed on enx0050b68b74e4: Network is down
Aug 23 20:50:24 $HOSTNAME wpa_supplicant[1290]: nl80211: Ignore RTM_DELLINK event for foreign ifindex 5
Aug 23 20:50:24 $HOSTNAME NetworkManager[84922]: <info>  [1535050224.6517] policy: set 'Auto $WIFI_SSID' (
wlp4s0) as default for IPv4 routing and DNS
Aug 23 20:50:24 $HOSTNAME NetworkManager[84922]: <info>  [1535050224.6519] device (enx0050b68b74e4): state change: activated -> unmanaged (reason 'removed', sys-iface-state: 'removed')
Aug 23 20:50:24 $HOSTNAME NetworkManager[84922]: <info>  [1535050224.6697] dhcp4 (enx0050b68b74e4): canceled DHCP transaction, DHCP client pid 89771
Aug 23 20:50:24 $HOSTNAME NetworkManager[84922]: <info>  [1535050224.6698] dhcp4 (enx0050b68b74e4): state changed bound -> done
Aug 23 20:50:24 $HOSTNAME NetworkManager[84922]: <info>  [1535050224.6700] dhcp6 (enx0050b68b74e4): canceled DHCP transaction
Aug 23 20:50:24 $HOSTNAME NetworkManager[113553]: <info>  [1535050224.8528] NetworkManager (version 1.12.0) is starting... (after a restart)

I've checked another instance of the crash and the log looks similar, so it probably is related to me unplugging the laptop and NM switching from ethernet as default to wifi as default connection.

I see it crashes shortly after some canceled dhcp6 transaction. My network only has IPv6 locally, so I don't have global IPv6 addresses (the Fritz!Box doesn't support it sadly).

Let me know if you need more information.
Comment 4 Beniamino Galvani 2018-09-13 12:56:30 UTC
Created attachment 373639 [details] [review]
[PATCH] dns: dnsmasq: avoid crash when no reverse domains exist
Comment 5 Beniamino Galvani 2018-09-13 12:59:05 UTC
Created attachment 373640 [details] [review]
[PATCH v2] dns: dnsmasq: avoid crash when no reverse domains exist

(v2: added Fixes: tag to commit message)
Comment 6 Thomas Haller 2018-09-13 13:02:56 UTC
(In reply to Beniamino Galvani from comment #5)
> Created attachment 373640 [details] [review] [review]
> [PATCH v2] dns: dnsmasq: avoid crash when no reverse domains exist
> 
> (v2: added Fixes: tag to commit message)

lgtm
Comment 7 Beniamino Galvani 2018-09-13 13:10:52 UTC
Applied to master:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f0c075f05082e4c77fac75ad06d303e7538e4fc7

and nm-1-14, nm-1-12 branches.
Comment 8 Markus Teich 2018-09-13 13:40:24 UTC
Thanks!
Comment 9 Michael Schaller 2018-09-14 08:05:02 UTC
Thanks. :-)