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<jhrozek(a)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.