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?= <pbrez...@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 89149aac8a7632f0344b042ea4d76f4d3de49eb3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 30 Jun 2014 11:23:21 +0200
Subject: [PATCH] sbus_request: fix potential NULL dereference
---
src/sbus/sssd_dbus_properties.c | 4 +---
src/sbus/sssd_dbus_request.c | 11 +++++++++--
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/sbus/sssd_dbus_properties.c b/src/sbus/sssd_dbus_properties.c
index 703f6eb81d8d788b7e9e46d68265b4e6136c0089..c6bdffda72b5a02f352cdeb27ac75b30bf07d6ba 100644
--- a/src/sbus/sssd_dbus_properties.c
+++ b/src/sbus/sssd_dbus_properties.c
@@ -358,9 +358,7 @@ dispatch_properties_get_all(struct sbus_connection *conn,
"No such interface"));
}
- sbus_request_invoke_or_finish(req, NULL,
- intf->instance_data,
- meta->invoker_get_all);
+ sbus_request_invoke_or_finish(req, NULL, NULL, meta->invoker_get_all);
return EOK;
}
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
index 2852f87d833d448e330fb36525083c4c8eb78605..7729d4e0d7bf6e517e2efce4dbeb064f6f471b87 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 (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;
}
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");
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
