On (30/06/14 13:23), Pavel Březina wrote: >On 06/30/2014 12:30 PM, Lukas Slebodnik wrote: >>On (30/06/14 11:24), Pavel Březina wrote: >>>On 06/26/2014 07:28 AM, Lukas Slebodnik wrote: >>>>ehlo, >>>> >>>>there is a bunch of error reported from static analysers >>>>Dereferencing a pointer that might be null "error" when calling >>>>"sbus_request_fail_and_finish" >>>> >>>>similar problem is with dereferencing NULL pointer in function >>>>sbus_request_invoke_or_finish. >>>> >>>>These situation can happen mostly with handling errors. >>>> >>>>LS >>> >>>Hi, >>>the attached patch should fix it. >>> >> >>>From d021b2302ff64798497cc08969fa328183e3063e Mon Sep 17 00:00:00 2001 >>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> >>>Date: Mon, 30 Jun 2014 11:23:21 +0200 >>>Subject: [PATCH] sbus_request: fix potential NULL dereference >>> >>>--- >>>src/sbus/sssd_dbus_request.c | 11 +++++++++-- >>>1 file changed, 9 insertions(+), 2 deletions(-) >>> >>>diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c >>>index >>>2852f87d833d448e330fb36525083c4c8eb78605..c39a6a4afc4af36c044995ad30bfd2c32fe32d8b >>> 100644 >>>--- a/src/sbus/sssd_dbus_request.c >>>+++ b/src/sbus/sssd_dbus_request.c >>>@@ -63,10 +63,12 @@ sbus_request_invoke_or_finish(struct sbus_request >>>*dbus_req, >>> DBusError error; >>> int ret; >>> >>>- if (invoker_fn) { >>>+ if (handler_fn != NULL && invoker_fn != NULL) { >>> ret = invoker_fn(dbus_req, handler_fn); >>>- } else { >>>+ } else if (handler_fn != NULL) { >>> ret = handler_fn(dbus_req, handler_data); >>>+ } else { >>>+ ret = EINVAL; >>> } >>I agree it is better to return EINVAL instead of crash, but I wonder why NULL >>is used as hadler_fn and handler data isn't NULL/ >>Probably it was not clear from very long list of warnings in my 1st mail. >> >>src/sbus/sssd_dbus_properties.c:361: sbus_request_invoke_or_finish(req, >>NULL, >>src/sbus/sssd_dbus_properties.c-362- >>intf->instance_data, >>src/sbus/sssd_dbus_properties.c-363- >>meta->invoker_get_all); >>src/sbus/sssd_dbus_properties.c-364- return EOK; >>src/sbus/sssd_dbus_properties.c-365-} > >I guess it is a relic from copy and paste, since invokers access handler data >directly through sbus_request. I think it would be more clear to also pass >handler data into invoker together with handler_fn. > >Also it may be good idea to create a special invoker type for getAll, since >getAll invokers doesn't use handler_fn paramater at all. > >Is it worth a ticket? > >I am sending a new patch since I broke getAll calls. > >>> >>> switch(ret) { >>>@@ -313,6 +315,11 @@ int sbus_request_fail_and_finish(struct sbus_request >>>*dbus_req, >>> DBusMessage *reply; >>> int ret; >>> >>>+ if (error == NULL) { >>>+ sbus_request_finish(dbus_req, NULL); >>>+ return ENOMEM; >>>+ } >>>+ >>> reply = dbus_message_new_error(dbus_req->message, error->name, >>> error->message); >>> if (!reply) { >>> DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus >>> message\n"); >> >>LS
>From 89149aac8a7632f0344b042ea4d76f4d3de49eb3 Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> >Date: Mon, 30 Jun 2014 11:23:21 +0200 >Subject: [PATCH] sbus_request: fix potential NULL dereference > >--- FORWARD_NULL: -6 NULL_RETURNS: -49 Good job :-) GetAll works fine and also other dbus methods. ACK LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
