On 04/27/2016 11:13 AM, Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 01:20:14PM +0200, Pavel Březina wrote:
On 04/20/2016 11:56 AM, Jakub Hrozek wrote:
Hi Pavel,
can you check if this is the right thing to do for methods that parse
arguments on their own?
To reproduce, it was enough to run:
sudo dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
org.freedesktop.sssd.infopipe.GetUserAttr string:admin
string:gecos
instead of the proper:
sudo dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
org.freedesktop.sssd.infopipe.GetUserAttr string:admin
array:string:gecos
0001-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
From 220b9124e81961a3febe814a0cb70d4fcfc2ade2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek<jhro...@redhat.com>
Date: Wed, 20 Apr 2016 11:54:31 +0200
Subject: [PATCH] IFP: Do not crash on invalid arguments to GetUserAttr
---
src/responder/ifp/ifpsrv_cmd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
index
399e83e0255700df5a24491a3eb33b4f92040ca7..43dbce53c0395edfd7e8e6d87b609c52e835eaa0
100644
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -117,7 +117,7 @@ ifp_user_get_attr_unpack_msg(struct ifp_attr_req *attr_req)
DBUS_TYPE_INVALID);
if (parsed == false) {
DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
- return EOK; /* handled */
+ return EINVAL;
}
/* Copy the attributes to maintain memory hierarchy with talloc */
-- 2.4.11
An sbus method handler returns EOK if the message was handled (reply was
sent) even if this was an error case. See:
void
sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
sbus_msg_handler_fn handler_fn,
void *handler_data,
sbus_method_invoker_fn invoker_fn)
{
DBusError error;
int ret;
if (invoker_fn != NULL) {
ret = invoker_fn(dbus_req, handler_fn);
} else if (handler_fn != NULL) {
ret = handler_fn(dbus_req, handler_data);
} else {
ret = EINVAL;
}
switch(ret) {
case EOK:
return;
^ message was handled, therefore we do not do anything else here
case ENOMEM:
DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory handling DBus message\n");
sbus_request_finish(dbus_req, NULL);
break;
default:
dbus_error_init(&error);
dbus_set_error_const(&error, DBUS_ERROR_FAILED, INTERNAL_ERROR);
sbus_request_fail_and_finish(dbus_req, &error);
break;
^ otherwise we report error
}
}
Because sbus_request is freed when you sent reply over
sbus_request_finish/fail_and_finish or whatever (which happens internally
when the params are invalid) you access freed memory in the default case if
!EOK is returned.
This is exactly what I ran into.
So you can return EINVAL in ifp_user_get_attr_unpack_msg but you need to
return EOK from ifp_user_get_attr in this case. Maybe we can create a custom
code ERR_SBUS_MESSAGE_HANDLED? for this.
OK, done. I inspected all calls of sbus_request_parse_or_finish() and I
didn't see any other place to use this error code, but it would be nice
if you checked as well during the code review.
Can you also extend sbus_request_invoke_or_finish() to treat
ERR_SBUS_REQUEST_HANDLED the same way as EOK? I.e.
case EOK:
case ERR_SBUS_REQUEST_HANDLED:
return;
This way you don't have to translate the new error code back to EOK.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
