4:56 a.m.
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
6:20 a.m.
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.
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.
4:13 a.m.
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.
4:47 a.m.
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.
4:38 a.m.
On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
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.
Sorry, I totally forgot about these patches.
Here you go..
5:06 a.m.
On 05/05/2016 11:38 AM, Jakub Hrozek wrote:
On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
> >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.
Sorry, I totally forgot about these patches.
Here you go..
0001-UTIL-Add-ERR_SBUS_REQUEST_HANDLED.patch
From d3b578dd84acd327f0f623ddb835cd031480bb0a Mon Sep 17 00:00:00 2001
From: Jakub Hrozek<jhrozek(a)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
0002-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
From 567b8dc9bae4f5a83bacfccc310376ffc27d7ae8 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 2/2] IFP: Do not crash on invalid arguments to GetUserAttr
---
src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
src/sbus/sssd_dbus_request.c | 1 +
2 files changed, 6 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 */
}
You can remove this hunk and stick with:
if (ret != EOK) {
return ret;
}
Since ERR_SBUS_REQUEST_HANDLED is now returned and another reply will
not be sent thanks to following hunks.
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 */
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
index aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912
100644
--- a/src/sbus/sssd_dbus_request.c
+++ b/src/sbus/sssd_dbus_request.c
@@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
}
switch(ret) {
+ case ERR_SBUS_REQUEST_HANDLED:
I think this should be in a separate patch, or squashed to the first one.
case EOK:
return;
case ENOMEM:
-- 2.4.11
5:34 a.m.
On Tue, May 10, 2016 at 12:06:39PM +0200, Pavel Březina wrote:
On 05/05/2016 11:38 AM, Jakub Hrozek wrote:
> On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
> > >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.
> Sorry, I totally forgot about these patches.
>
> Here you go..
>
>
> 0001-UTIL-Add-ERR_SBUS_REQUEST_HANDLED.patch
>
>
> From d3b578dd84acd327f0f623ddb835cd031480bb0a Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek<jhrozek(a)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
>
>
> 0002-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
>
>
> From 567b8dc9bae4f5a83bacfccc310376ffc27d7ae8 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 2/2] IFP: Do not crash on invalid arguments to GetUserAttr
>
> ---
> src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
> src/sbus/sssd_dbus_request.c | 1 +
> 2 files changed, 6 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 */
> }
You can remove this hunk and stick with:
if (ret != EOK) {
return ret;
}
Since ERR_SBUS_REQUEST_HANDLED is now returned and another reply will not be
sent thanks to following hunks.
>
> 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 */
> diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
> index
aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912 100644
> --- a/src/sbus/sssd_dbus_request.c
> +++ b/src/sbus/sssd_dbus_request.c
> @@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
> }
>
> switch(ret) {
> + case ERR_SBUS_REQUEST_HANDLED:
I think this should be in a separate patch, or squashed to the first one.
Thanks both done. I don't think we need a separate one-liner.
5:53 a.m.
On 05/10/2016 12:34 PM, Jakub Hrozek wrote:
On Tue, May 10, 2016 at 12:06:39PM +0200, Pavel Březina wrote:
> On 05/05/2016 11:38 AM, Jakub Hrozek wrote:
>> On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
>>>> 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.
>> Sorry, I totally forgot about these patches.
>>
>> Here you go..
>>
>>
>> 0001-UTIL-Add-ERR_SBUS_REQUEST_HANDLED.patch
>>
>>
>> From d3b578dd84acd327f0f623ddb835cd031480bb0a Mon Sep 17 00:00:00 2001
>> From: Jakub Hrozek<jhrozek(a)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
>>
>>
>> 0002-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
>>
>>
>> From 567b8dc9bae4f5a83bacfccc310376ffc27d7ae8 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 2/2] IFP: Do not crash on invalid arguments to GetUserAttr
>>
>> ---
>> src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
>> src/sbus/sssd_dbus_request.c | 1 +
>> 2 files changed, 6 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 */
>> }
>
> You can remove this hunk and stick with:
> if (ret != EOK) {
> return ret;
> }
>
> Since ERR_SBUS_REQUEST_HANDLED is now returned and another reply will not be
> sent thanks to following hunks.
>
>>
>> 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 */
>> diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
>> index
aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912 100644
>> --- a/src/sbus/sssd_dbus_request.c
>> +++ b/src/sbus/sssd_dbus_request.c
>> @@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
>> }
>>
>> switch(ret) {
>> + case ERR_SBUS_REQUEST_HANDLED:
>
> I think this should be in a separate patch, or squashed to the first one.
Thanks both done. I don't think we need a separate one-liner.
Ack.
3:28 a.m.
On Tue, May 10, 2016 at 12:53:08PM +0200, Pavel Březina wrote:
On 05/10/2016 12:34 PM, Jakub Hrozek wrote:
> On Tue, May 10, 2016 at 12:06:39PM +0200, Pavel Březina wrote:
> > On 05/05/2016 11:38 AM, Jakub Hrozek wrote:
> > > On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
> > > > > 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.
> > > Sorry, I totally forgot about these patches.
> > >
> > > Here you go..
> > >
> > >
> > > 0001-UTIL-Add-ERR_SBUS_REQUEST_HANDLED.patch
> > >
> > >
> > > From d3b578dd84acd327f0f623ddb835cd031480bb0a Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek<jhrozek(a)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
> > >
> > >
> > > 0002-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
> > >
> > >
> > > From 567b8dc9bae4f5a83bacfccc310376ffc27d7ae8 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 2/2] IFP: Do not crash on invalid arguments to
GetUserAttr
> > >
> > > ---
> > > src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
> > > src/sbus/sssd_dbus_request.c | 1 +
> > > 2 files changed, 6 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 */
> > > }
> >
> > You can remove this hunk and stick with:
> > if (ret != EOK) {
> > return ret;
> > }
> >
> > Since ERR_SBUS_REQUEST_HANDLED is now returned and another reply will not be
> > sent thanks to following hunks.
> >
> > >
> > > 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 */
> > > diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
> > > index
aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912 100644
> > > --- a/src/sbus/sssd_dbus_request.c
> > > +++ b/src/sbus/sssd_dbus_request.c
> > > @@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request
*dbus_req,
> > > }
> > >
> > > switch(ret) {
> > > + case ERR_SBUS_REQUEST_HANDLED:
> >
> > I think this should be in a separate patch, or squashed to the first one.
>
> Thanks both done. I don't think we need a separate one-liner.
Ack.
Thank you for the careful review. Pushed to master:
e8474ac0be7e81c0ca54eb09e2fef42595602945
c30b7a1931211fdcae0564551a7625cc4f6dee9f
and sssd-1-13:
7ff6858b18fb463bc446797aa860960d5165fe9e
406a7e5b731ae79084dce00021e01ebe7b7d724a
3:32 a.m.
On Wed, May 11, 2016 at 10:28:16AM +0200, Jakub Hrozek wrote:
On Tue, May 10, 2016 at 12:53:08PM +0200, Pavel Březina wrote:
> On 05/10/2016 12:34 PM, Jakub Hrozek wrote:
> > On Tue, May 10, 2016 at 12:06:39PM +0200, Pavel Březina wrote:
> > > On 05/05/2016 11:38 AM, Jakub Hrozek wrote:
> > > > On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
> > > > > > 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.
> > > > Sorry, I totally forgot about these patches.
> > > >
> > > > Here you go..
> > > >
> > > >
> > > > 0001-UTIL-Add-ERR_SBUS_REQUEST_HANDLED.patch
> > > >
> > > >
> > > > From d3b578dd84acd327f0f623ddb835cd031480bb0a Mon Sep 17 00:00:00
2001
> > > > From: Jakub Hrozek<jhrozek(a)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
> > > >
> > > >
> > > > 0002-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
> > > >
> > > >
> > > > From 567b8dc9bae4f5a83bacfccc310376ffc27d7ae8 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 2/2] IFP: Do not crash on invalid arguments to
GetUserAttr
> > > >
> > > > ---
> > > > src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
> > > > src/sbus/sssd_dbus_request.c | 1 +
> > > > 2 files changed, 6 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 */
> > > > }
> > >
> > > You can remove this hunk and stick with:
> > > if (ret != EOK) {
> > > return ret;
> > > }
> > >
> > > Since ERR_SBUS_REQUEST_HANDLED is now returned and another reply will not
be
> > > sent thanks to following hunks.
> > >
> > > >
> > > > 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 */
> > > > diff --git a/src/sbus/sssd_dbus_request.c
b/src/sbus/sssd_dbus_request.c
> > > > index
aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912 100644
> > > > --- a/src/sbus/sssd_dbus_request.c
> > > > +++ b/src/sbus/sssd_dbus_request.c
> > > > @@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request
*dbus_req,
> > > > }
> > > >
> > > > switch(ret) {
> > > > + case ERR_SBUS_REQUEST_HANDLED:
> > >
> > > I think this should be in a separate patch, or squashed to the first one.
> >
> > Thanks both done. I don't think we need a separate one-liner.
>
>
> Ack.
Thank you for the careful review. Pushed to master:
e8474ac0be7e81c0ca54eb09e2fef42595602945
c30b7a1931211fdcae0564551a7625cc4f6dee9f
and sssd-1-13:
7ff6858b18fb463bc446797aa860960d5165fe9e
406a7e5b731ae79084dce00021e01ebe7b7d724a
Sorry, I forgot the CI link:
http://sssd-ci.duckdns.org/logs/job/43/11/summary.html
2906
days inactive
2927
days old
sssd-devel@lists.fedorahosted.org
9 comments
2 participants
participants (2)
-
Jakub Hrozek -
Pavel Březina