Bug 25912

I believe the most appropriate place to kludge around this bug is libdrm's xf86drmMode.c, in the drmModeGetConnector function - where the bad data is coming from.

More fundamentally though, the double ioctl call without some kind of locking against mode-list changes between them is flawed, and the root cause of this bug.

Something like this would kludge around it, abiet reporting no modes in the case of error. I guess in theory, the same bug could affect properties, and any other dual-ioctl based query.

--- libdrm-2.4.17+git20091230.c5c503b5.orig/xf86drmMode.c
+++ libdrm-2.4.17+git20091230.c5c503b5/xf86drmMode.c
@@ -373,6 +373,8 @@
 {
 	struct drm_mode_get_connector conn;
 	drmModeConnectorPtr r = NULL;
+	int num_modes_first;
+	int num_modes_second;
 
 	conn.connector_id = connector_id;
 	conn.connector_type_id = 0;
@@ -388,6 +390,8 @@
 	if (drmIoctl(fd, DRM_IOCTL_MODE_GETCONNECTOR, &conn))
 		return 0;
 
+	num_modes_first = conn.count_modes;
+
 	if (conn.count_props) {
 		conn.props_ptr = VOID2U64(drmMalloc(conn.count_props*sizeof(uint32_t)));
 		conn.prop_values_ptr = VOID2U64(drmMalloc(conn.count_props*sizeof(uint64_t)));
@@ -402,6 +406,13 @@
 	if (drmIoctl(fd, DRM_IOCTL_MODE_GETCONNECTOR, &conn))
 		goto err_allocs;
 
+	num_modes_second = conn.count_modes;
+
+	if (num_modes_first != num_modes_second) {
+		fprintf (stderr, "DRM PROBLEM: OH DEAR Num modes before (%i) is not equal to that after (%i)\n",
+		         num_modes_first, num_modes_second);
+	}
+
 	if(!(r = drmMalloc(sizeof(*r)))) {
 		goto err_allocs;
 	}
@@ -418,7 +429,16 @@
 	r->count_props  = conn.count_props;
 	r->props        = drmAllocCpy(U642VOID(conn.props_ptr), conn.count_props, sizeof(uint32_t));
 	r->prop_values  = drmAllocCpy(U642VOID(conn.prop_values_ptr), conn.count_props, sizeof(uint64_t));
-	r->modes        = drmAllocCpy(U642VOID(conn.modes_ptr), conn.count_modes, sizeof(struct drm_mode_modeinfo));
+
+	if (num_modes_first >= num_modes_second) {
+		r->modes = drmAllocCpy(U642VOID(conn.modes_ptr), conn.count_modes, sizeof(struct drm_mode_modeinfo));
+	} else {
+		r->modes = NULL;
+	}
+	if (r->count_modes != 0 && r->modes == NULL) {
+		fprintf (stderr, "DRM PROBLEM: Copying modes failed.. zeroing count\n");
+		r->count_modes = 0;
+	}
 	r->count_encoders = conn.count_encoders;
 	r->encoders     = drmAllocCpy(U642VOID(conn.encoders_ptr), conn.count_encoders, sizeof(uint32_t));
 	r->connector_type  = conn.connector_type;

*** Bug 25610 has been marked as a duplicate of this bug. ***

I don't think the properties (and friends) are similarly affected by hotplugging -- i.e. I think they are fixed upon initialisation.

commit d1308f4fe7f94aae51ca9f70947aea8e09597f37
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jan 6 15:19:25 2010 +0000

    modes: Retry GETRESOURCES if a hotplug event occurs between the two ioctls
    
    Peter Clifton hit an issue whereby he had a spurious TV hotplug event
    that occurred between the two GETRESOURCES ioctls that caused the kernel
    to skip filling one array and led to a crash (as the size of the
    allocated and initialised block of memory differed from the reported
    size).
    
    Fixes: http://bugs.freedesktop.org/show_bug.cgi?id=25912
           Crash whilst probing modes
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Reported-by: Peter Clifton <pcjc2@cam.ac.uk>

Thanks Chris!

I'll let you know if I encounter any problems with that patch applied to libdrm. (WIll have to unpatch the drm driver to remove my (possible) fix for the TV-out misdetect issue).

Btw, I don't think the issue had anything to do with hotplug events in my case.. the TV is probed on each ioctl call, and either could detect attached / detached. Somewhat un-necessary in terms of time required to probe (and xrandr to return), but there you go.

Created attachment 32480 [details] [review]
Additional patch required to fix it for me..

This takes Chris's changes for the GETRESOURCES ioctl, and apply them to the one which was crashing for me, GETCONNECTOR

Re-opening, as the committed fix is not sufficient.

Similar error?

Yes, with just your patch (and my TV-probe fix removed), Xorg crashes like clockwork. Also, I spotted plymouthd hitting a segfault in libdrm.

Ubunut's core dump isn't working properly at the moment unfortunately though, so its not so easy to get a retrace.

With my patch as well, I haven't seen it crash (not doing xrandr at least... the damned lid is another matter!)

Peter, thanks for the patch. Pushed:

commit 04f90a44709a48fb932ea954011cb551659bf246
Author: Peter Clifton <pcjc2@cam.ac.uk>
Date:   Wed Jan 6 20:44:11 2010 +0000

    modes: Retry GETCONNECTOR if a hotplug event occurs between the two ioctls

Hopefully this puts the bug to rest...