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.
>From d6819e335eea0da2f3c9fef077ff00e663fa44a2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 27 Apr 2016 11:11:31 +0200
Subject: [PATCH 1/2] UTIL: Add ERR_SBUS_REQUEST_HANDLED
In most cases when sbus request parsing finishes, the request is handled
internally and a reply is sent to the caller. However, in handlers that
are parsed and handled completely manually, we might want to be notified
about this case so that the called of sbus_request_parse_or_finish()
aborts the request and doesn't proceed with using the sbus request which
is already freed internally in sbus_request_parse_or_finish().
---
src/util/util_errors.c | 1 +
src/util/util_errors.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/util/util_errors.c b/src/util/util_errors.c
index
59ae63ab8d6e834a772349b162bf282f9a4f1c72..c998e14c26e43c3cd6a5a060bb6f74698b9e93ae
100644
--- a/src/util/util_errors.c
+++ b/src/util/util_errors.c
@@ -84,6 +84,7 @@ struct err_string error_to_str[] = {
{ "Subdomain is inactive" }, /* ERR_SUBDOM_INACTIVE */
{ "Account is locked" }, /* ERR_ACCOUNT_LOCKED */
{ "AD renewal child failed" }, /* ERR_RENEWAL_CHILD */
+ { "SBUS request already handled" }, /* ERR_SBUS_REQUEST_HANDLED */
{ "ERR_LAST" } /* ERR_LAST */
};
diff --git a/src/util/util_errors.h b/src/util/util_errors.h
index
05791f2f08f107a8b4830b810b8826983763174f..c0d9622a431a9946fdfa5e5c60ecf7b9e1ae66a5
100644
--- a/src/util/util_errors.h
+++ b/src/util/util_errors.h
@@ -106,6 +106,7 @@ enum sssd_errors {
ERR_SUBDOM_INACTIVE,
ERR_ACCOUNT_LOCKED,
ERR_RENEWAL_CHILD,
+ ERR_SBUS_REQUEST_HANDLED,
ERR_LAST /* ALWAYS LAST */
};
--
2.4.11
>From 22e2710e814b9f6239adb137db47cc4ae3fe507a Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 20 Apr 2016 11:54:31 +0200
Subject: [PATCH 2/2] IFP: Do not crash on invalid arguments to GetUserAttr
---
src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
index
399e83e0255700df5a24491a3eb33b4f92040ca7..b1e8f80f22d535b5e2579ac25b39b61b0fa5b33b
100644
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -82,8 +82,10 @@ int ifp_user_get_attr(struct sbus_request *dbus_req, void
*data)
attr_req->ireq = ireq;
ret = ifp_user_get_attr_unpack_msg(attr_req);
- if (ret != EOK) {
- return ret; /* handled internally */
+ if (ret == ERR_SBUS_REQUEST_HANDLED) {
+ return EOK; /* handled internally */
+ } else if (ret != EOK) {
+ return ret; /* internal error */
}
DEBUG(SSSDBG_FUNC_DATA,
@@ -117,7 +119,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 ERR_SBUS_REQUEST_HANDLED;
}
/* Copy the attributes to maintain memory hierarchy with talloc */
--
2.4.11
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
