Hans de Goede <[email protected]> writes: > Looks like the value of ODEV_ATTRIB_DRIVER was not updated when the patch > adding it got rebased on top of a newer server version.
This points out some weaknesses in the config_odev API; it shouldn't be possible to crash the server in this way. Here's a patch which tries to make this API robust against future errors and emit error messages that should help track down incorrect usage. I am going to insist that the API be made robust (either with this patch or something which has the desired effect; I haven't done more than compile this one) while the existing bug is still present so that we can verify that the code now survives this abuse without crashing. After that works, I can merge in both that and the actual fix.
From b0f54b5a46394e92d4c98f1463b751c01723d759 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Fri, 11 Jul 2014 02:14:23 -0700 Subject: [PATCH] config: Make OdevAttribute API robust against mis-using of IDs An OdevAttribute can hold either an integer or string value, but there are a few places which don't check to make sure the right type is in use. * When adding a string value, if an existing int value is sharing the same attrib_id, a call to free will be made with an invalid pointer. * When freeing attributes, the major/minor device numbers and FD are pulled out of the list, but no typecheck is made to ensure that the values are actually stored as integers. This fix adds a new type, 'ODEV_ATTRIB_NONE' which is given to newly allocated OdevAttribute structs. Then, the functions which add ints and strings values can check to make sure they always free previous string values. These functions also now emit error messages if the caller replaces an int with a string and vice-versa. Signed-off-by: Keith Packard <[email protected]> --- config/config.c | 25 +++++++++++++++++++------ include/hotplug.h | 4 ++-- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/config/config.c b/config/config.c index 5514516..94270b1 100644 --- a/config/config.c +++ b/config/config.c @@ -132,7 +132,7 @@ config_odev_allocate_attribute_list(void) { struct OdevAttributes *attriblist; - attriblist = XNFalloc(sizeof(struct OdevAttributes)); + attriblist = XNFcalloc(sizeof(struct OdevAttributes)); xorg_list_init(&attriblist->list); return attriblist; } @@ -167,6 +167,7 @@ config_odev_find_or_add_attribute(struct OdevAttributes *attribs, int attrib) oa = XNFcalloc(sizeof(struct OdevAttribute)); oa->attrib_id = attrib; + oa->attrib_type = ODEV_ATTRIB_NONE; xorg_list_append(&oa->member, &attribs->list); return oa; @@ -179,7 +180,12 @@ config_odev_add_attribute(struct OdevAttributes *attribs, int attrib, struct OdevAttribute *oa; oa = config_odev_find_or_add_attribute(attribs, attrib); - free(oa->attrib_name); + if (oa->attrib_type == ODEV_ATTRIB_STRING) + free(oa->attrib_name); + else if (oa->attrib_type == ODEV_ATTRIB_INT) { + LogMessage(X_ERROR, "Error %s called replacing attrib %d int %d with string %s\n", + __func__, attrib, oa->attrib_value, attrib_name); + } oa->attrib_name = XNFstrdup(attrib_name); oa->attrib_type = ODEV_ATTRIB_STRING; return TRUE; @@ -192,6 +198,11 @@ config_odev_add_int_attribute(struct OdevAttributes *attribs, int attrib, struct OdevAttribute *oa; oa = config_odev_find_or_add_attribute(attribs, attrib); + if (oa->attrib_type == ODEV_ATTRIB_STRING) { + LogMessage(X_ERROR, "Error %s called replacing attrib %d string %s with int %d\n", + __func__, attrib, oa->attrib_name, attrib_value); + free(oa->attrib_name); + } oa->attrib_value = attrib_value; oa->attrib_type = ODEV_ATTRIB_INT; return TRUE; @@ -239,10 +250,12 @@ config_odev_free_attributes(struct OdevAttributes *attribs) int major = 0, minor = 0, fd = -1; xorg_list_for_each_entry_safe(iter, safe, &attribs->list, member) { - switch (iter->attrib_id) { - case ODEV_ATTRIB_MAJOR: major = iter->attrib_value; break; - case ODEV_ATTRIB_MINOR: minor = iter->attrib_value; break; - case ODEV_ATTRIB_FD: fd = iter->attrib_value; break; + if (iter->attrib_type == ODEV_ATTRIB_INT) { + switch (iter->attrib_id) { + case ODEV_ATTRIB_MAJOR: major = iter->attrib_value; break; + case ODEV_ATTRIB_MINOR: minor = iter->attrib_value; break; + case ODEV_ATTRIB_FD: fd = iter->attrib_value; break; + } } xorg_list_del(&iter->member); if (iter->attrib_type == ODEV_ATTRIB_STRING) diff --git a/include/hotplug.h b/include/hotplug.h index c4268a0..b3d12e9 100644 --- a/include/hotplug.h +++ b/include/hotplug.h @@ -32,7 +32,7 @@ extern _X_EXPORT void config_pre_init(void); extern _X_EXPORT void config_init(void); extern _X_EXPORT void config_fini(void); -enum { ODEV_ATTRIB_STRING, ODEV_ATTRIB_INT }; +enum OdevAttributeType { ODEV_ATTRIB_NONE, ODEV_ATTRIB_STRING, ODEV_ATTRIB_INT }; struct OdevAttribute { struct xorg_list member; @@ -41,7 +41,7 @@ struct OdevAttribute { char *attrib_name; int attrib_value; }; - int attrib_type; + enum OdevAttributeType attrib_type; }; struct OdevAttributes { -- 2.0.0.rc4
-- [email protected]
pgpcI9gxH5ATm.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
