On Wed, May 13, 2015 at 04:51:53PM +0200, Pavel Březina wrote:
> On 05/11/2015 05:58 PM, Jakub Hrozek wrote:
>> Thanks for the patches. They work quite well!
>>
>> One bug I found is that if you query a nonexistant object path with
>> GetAll, then all subsequent queries block. Maybe we don't finish some
>> request on error? This is what I tried:
>> [jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe/Users/redhat_2ecom/10327
org.freedesktop.DBus.Properties.GetAll string:org.freedesktop.sssd.infopipe.Users.User
>> Error org.freedesktop.DBus.Error.NoReply: Message did not receive a reply
(timeout by message bus)
>> [jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe/Users/linux_2etest/465400000
org.freedesktop.DBus.Properties.GetAll string:org.freedesktop.sssd.infopipe.Users.User
>> Error org.freedesktop.DBus.Error.TimedOut: Activation of
org.freedesktop.sssd.infopipe timed out
>>
>> The first objectpath is a copy-n-paste error from a different machine,
>> the second is legitimate. All works well unless I use the nonexistant path.
>
> Container wasn't closed properly and it broke the bus.
>
> diff --git a/src/sbus/sssd_dbus_invokers.c b/src/sbus/sssd_dbus_invokers.c
> index b2f0de0..f4d4ba3 100644
> --- a/src/sbus/sssd_dbus_invokers.c
> +++ b/src/sbus/sssd_dbus_invokers.c
> @@ -336,6 +336,12 @@ int sbus_invoke_get_aDOsasDE(DBusMessageIter *iter,
> /* iterate over keys */
>
> if (table == NULL) {
> + dbret = dbus_message_iter_close_container(iter, &it_array);
> + if (!dbret) {
> + ret = EIO;
> + goto done;
> + }
> +
> ret = EOK;
> goto done;
> }
>
>
> New patches are attached.
I'm still testing functionality, but Coverity reported dead code:
Error: DEADCODE (CWE-561):
sssd-1.12.90/src/responder/ifp/ifp_groups.c:325: cond_notnull: Condition "req ==
NULL", taking false branch. Now the value of "req" is not
"NULL".
sssd-1.12.90/src/responder/ifp/ifp_groups.c:357: notnull: At condition "req ==
NULL", the value of "req" cannot be "NULL".
sssd-1.12.90/src/responder/ifp/ifp_groups.c:357: dead_error_condition: The condition
"req == NULL" cannot be true.
sssd-1.12.90/src/responder/ifp/ifp_groups.c:358: dead_error_begin: Execution cannot reach
this statement: "ret = 12;".
# 356| domain->name, name);
# 357| if (req == NULL) {
# 358|-> ret = ENOMEM;
# 359| goto immediately;
# 360| }
and ifp unit test is consistently failing for me:
FAIL: ifp_tests
===============
[==========] Running 6 test(s).
[ RUN ] ifp_test_req_create
[ OK ] ifp_test_req_create
[ RUN ] ifp_test_req_wrong_uid
[ OK ] ifp_test_req_wrong_uid
[ RUN ] test_el_to_dict
[ OK ] test_el_to_dict
[ RUN ] test_attr_acl
s2[i]
/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_ifp.c:214: error: Failure!
[ FAILED ] test_attr_acl
[ RUN ] test_attr_acl_ex
[ OK ] test_attr_acl_ex
[ RUN ] test_attr_allowed
[ OK ] test_attr_allowed
[==========] 6 test(s) run.
[ PASSED ] 5 test(s).
[ FAILED ] 1 test(s), listed below:
[ FAILED ] test_attr_acl
1 FAILED TEST(S)
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for the review. New patches are attached.