[SSSD] [PATCH] sbus_request: fix potential NULL dereference

Pavel Březina pbrezina at redhat.com
Mon Jun 30 11:23:58 UTC 2014


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?= <pbrezina at redhat.com>
>> 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
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sbus_request-fix-potential-NULL-dereference.patch
Type: text/x-patch
Size: 2095 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140630/32e1ca2b/attachment.bin>


More information about the sssd-devel mailing list