udev segfaults in udev_monitor_enable_receiving when netlink doesn't work

Bug #581527 reported by Stéphane Graber on 2010-05-17
22
This bug affects 3 people
Affects Status Importance Assigned to Milestone
rhythmbox (Ubuntu)
Undecided
Unassigned
Lucid
Undecided
Unassigned
Maverick
Undecided
Unassigned
udev (Ubuntu)
Low
Unassigned
Lucid
Undecided
Unassigned
Maverick
Low
Unassigned

Bug Description

Binary package hint: udev

In Ubuntu lucid running udev 151, it's possible to call udev_monitor_enable_receiving and have it to segfault when the kernel netlink doesn't work (for example with older kernels running a lucid container).

Rather than segfaulting, udev_monitor_enable_receiving should return a proper error code which would let the software handle that error rather than simply crashing.

Here's an example with rhythmbox:
-----
(rhythmbox:27375): GVFS-RemoteVolumeMonitor-WARNING **: invoking IsSupported() failed for remote volume monitor with dbus name org.gtk.Private.GduVolumeMonitor: org.freedesktop.DBus.Error.Spawn.ChildSignaled: Process /usr/lib/gvfs/gvfs-gdu-volume-monitor received signal 6
[New Thread 0x7fffe9ea2710 (LWP 27380)]
libudev: udev_monitor_new_from_netlink: error getting socket: Invalid argument
-----

At this point, rhythmbox segfaults and I get the following backtrace:
-----
#0 udev_monitor_enable_receiving (udev_monitor=0x0) at ../libudev/libudev-monitor.c:324
#1 0x00007ffff1d1cc6c in g_udev_client_constructed (object=0x9b9c00) at ../extras/gudev/gudevclient.c:225
#2 0x00007ffff38a1a58 in IA__g_object_newv (object_type=<value optimized out>, n_parameters=<value optimized out>, parameters=0x0) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1289
#3 0x00007ffff38a22ad in IA__g_object_new_valist (object_type=8563696, first_property_name=0x0, var_args=0x7fffffffc8e0) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1377
#4 0x00007ffff38a24f1 in IA__g_object_new (object_type=8563696, first_property_name=0x7ffff1d1df32 "subsystems") at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1095
#5 0x00007ffff1d1c8b2 in g_udev_client_new (subsystems=0x7fffffffc9f0) at ../extras/gudev/gudevclient.c:314
#6 0x00007ffff7aef89b in rb_removable_media_manager_init (mgr=0xa24cc0) at rb-removable-media-manager.c:395
#7 0x00007ffff38bc935 in IA__g_type_create_instance (type=<value optimized out>) at /build/buildd/glib2.0-2.24.0/gobject/gtype.c:1885
#8 0x00007ffff38a083c in g_object_constructor (type=0, n_construct_properties=0, construct_params=0xa2b180) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1396
#9 0x00007ffff38a1841 in IA__g_object_newv (object_type=<value optimized out>, n_parameters=1, parameters=<value optimized out>) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1261
#10 0x00007ffff38a22ad in IA__g_object_new_valist (object_type=8487728, first_property_name=0x0, var_args=0x7fffffffcde0) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1377
#11 0x00007ffff38a24f1 in IA__g_object_new (object_type=8487728, first_property_name=0x7ffff7b88dc7 "shell") at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1095
#12 0x00007ffff7adb0aa in construct_sources (shell=0x81c010) at rb-shell.c:1388
#13 0x00007ffff7adce6c in rb_shell_constructed (object=<value optimized out>) at rb-shell.c:1550
#14 0x00007ffff38a1a58 in IA__g_object_newv (object_type=<value optimized out>, n_parameters=<value optimized out>, parameters=0x0) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1289
#15 0x00007ffff38a22ad in IA__g_object_new_valist (object_type=8352528, first_property_name=0x0, var_args=0x7fffffffd170) at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1377
#16 0x00007ffff38a24f1 in IA__g_object_new (object_type=8352528, first_property_name=0x7ffff7b884ce "no-registration") at /build/buildd/glib2.0-2.24.0/gobject/gobject.c:1095
#17 0x00007ffff7ad8fd2 in rb_shell_new (no_registration=0, no_update=0, dry_run=0, rhythmdb=0x0, playlists=0x0) at rb-shell.c:1116
#18 0x0000000000403e22 in main (argc=1, argv=0x7fffffffd418) at main.c:272
-----

Manually editing udev_monitor_enable_receiving to return -1 makes rhythmbox to work properly.

My guess would be that the structure received as parameter to the udev_monitor_enable_receiving is invalid and somehow doesn't contain a sun.sun_family value making it to segfault. A fix would be to make that case return an error value instead of segfaulting.

Stéphane Graber (stgraber) wrote :

I know that using lucid in a container on top of an older kernel (in this case Hardy's) is not recommended and is known to fail.
Most users doing that know that, though I believe that if something can be fixed in udev to correctly fail rather than segfault, it's still worth fixing.

Please let me know if you need any additional information on that environment.

Stéphane Graber (stgraber) wrote :

Ok, so that patch does the trick.
I haven't been looking at the definition of the return codes so -1 might not be a good idea :)

Stéphane Graber (stgraber) wrote :

Here's a debdiff for Lucid.
It seems like Debian isn't using any kind of patching system, so that's a patch directly applied to upstream code + it's changelog entry targeted at lucid-proposed.

As for maverick, I believe that this bug should be fixed upstream and will then be pulled from a new udev in Debian without needing any ubuntu-specific release (as udev is currently synced from Debian).

Changed in udev (Ubuntu):
milestone: none → lucid-updates
milestone: lucid-updates → none
Changed in udev (Ubuntu Lucid):
milestone: none → lucid-updates
Stéphane Graber (stgraber) wrote :

Sorry, I don't agree with this patch.

This should be an assertion, rather than a check

Note that the bug here is clearly in rhythmbox, libudev correctly returns NULL for the monitor because there is no netlink socket - and rhythmbox failed to check for that, and blindly passed that NULL around assuming it was a valid object

Changed in udev (Ubuntu Lucid):
status: New → Won't Fix
Changed in udev (Ubuntu Maverick):
status: New → Triaged
importance: Undecided → Low

Won't Fix the lucid udev, the correct patch merely turns a segfault into an assertion error and will still terminate the process. Will send adding an assert in there upstream for maverick though

Opened the bug on rhythmbox for not checking the return value is not NULL before blindly passing it around

papukaija (papukaija) on 2010-07-26
tags: added: lucid maverick patch
Martin Pitt (pitti) on 2010-08-20
tags: removed: patch
Martin Pitt (pitti) wrote :

The current code in udev is quite inconsistent, though -- about half of the udev_monitor_*() functions return NULL if the passed monitor is NULL, the other half will crash.

Martin Pitt (pitti) wrote :

Looking at the stacktrace and rhythmbox code, it actually crashes in g_udev_client_new(). This is actually a gudev bug.

Changed in rhythmbox (Ubuntu Maverick):
status: New → Invalid
Changed in udev (Ubuntu Lucid):
milestone: lucid-updates → none
Changed in udev (Ubuntu Maverick):
assignee: nobody → Martin Pitt (pitti)
status: Triaged → In Progress
Changed in rhythmbox (Ubuntu Lucid):
status: New → Invalid
Martin Pitt (pitti) wrote :
Changed in udev (Ubuntu Maverick):
assignee: Martin Pitt (pitti) → nobody
status: In Progress → Fix Committed
Martin Pitt (pitti) wrote :

Stephane, if this is important to you, please feel free to backport it to lucid and reopen the lucid task. I don't object to it from an SRU POV, but I won't do it myself.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package udev - 161+git20100827-1

---------------
udev (161+git20100827-1) maverick; urgency=low

  * Merge fixes from trunk:
    - keymap: Fix Acer TravelMate 4720 (LP: #569815)
    - gudev: fix crash if netlink is not available (LP: #581527)
    - udev(7) manpage: Fix description of $attr (LP: #348513)
  * debian/changelog: Fix bug reference in previous upload.
  * debian/udev.{pre,post}inst: Remove pre-lucid upgrade code.
  * debian/udev.postinst: Drop obsolete /lib/udev/devices/sndstat symlink, OSS
    has gone from our kernels ages ago. (LP: #605443)
  * debian/udev.postinst, create_devices(): Drop devices which are handled by
    static_dev_create_links().
  * debian/udev.postinst, create_devices(): Drop devices which are handled by
    devtmpfs.
  * debian/rules: Work around gtk-doc not being able to work in a separate
    build tree; debian/rules prep already fixes $srcdir→$builddir, so copy
    the relevant source files into the build tree so that gtk-doc has
    something to scan for. (LP: #519670)
  * Add debian/local/hotplug.functions: Provides some helper functions which
    udev callouts can use. This is being used by usb-modeswitch, alsa, and
    other Debian packages. Copied from current udev sid package. (LP: #625110)
 -- Martin Pitt <email address hidden> Mon, 30 Aug 2010 11:21:43 +0200

Changed in udev (Ubuntu Maverick):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers