On Mon, May 18, 2015 at 11:56:29AM +0200, Pavel Březina wrote:
On 05/14/2015 04:34 PM, Jakub Hrozek wrote:
>On Thu, May 14, 2015 at 02:22:53PM +0200, Pavel Březina wrote:
>>On 05/14/2015 01:26 PM, Jakub Hrozek wrote:
>>>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.
>
>The patches are now fine, but I found a way to crash the ifp provider by
>looking up one of my users:
>[jhrozek@client] ~ $ [] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Users
org.freedesktop.sssd.infopipe.Users.FindByName string:seuser
>method return sender=:1.64 -> dest=:1.65 reply_serial=2
> object path
"/org/freedesktop/sssd/infopipe/Users/linux_2etest/465400006"
>[jhrozek@client] ~ $ [] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe/Users/linux_2etest/465400006
org.freedesktop.DBus.Properties.GetAll string:org.freedesktop.sssd.infopipe.Users.User
>Error org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes
include: the remote application did not send a reply, the message bus security policy
blocked the reply, the reply timeout expired, or the network connection was broken.
Thanks for the review, new patches are attached. This is the change:
>diff --git a/src/responder/ifp/ifp_users.c b/src/responder/ifp/ifp_users.c
>index d9e137d..26c1aa6 100644
>--- a/src/responder/ifp/ifp_users.c
>+++ b/src/responder/ifp/ifp_users.c
>@@ -477,6 +477,7 @@ void ifp_users_user_get_groups(struct sbus_request *sbus_req,
> struct ldb_message *user;
> struct ldb_result *res;
> const char **out;
>+ int num_groups;
> gid_t gid;
> errno_t ret;
> int i;
>@@ -515,12 +516,17 @@ void ifp_users_user_get_groups(struct sbus_request *sbus_req,
> return;
> }
>
>+ if (res->count == 0) {
>+ return;
>+ }
>+
> out = talloc_zero_array(sbus_req, const char *, res->count);
> if (out == NULL) {
> DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> return;
> }
>
>+ num_groups = 0;
> for (i = 0; i < res->count; i++) {
> gid = sss_view_ldb_msg_find_attr_as_uint64(domain, res->msgs[i],
> SYSDB_GIDNUM, 0);
>@@ -533,10 +539,12 @@ void ifp_users_user_get_groups(struct sbus_request *sbus_req,
> DEBUG(SSSDBG_CRIT_FAILURE, "ifp_groups_build_path()
failed\n");
> return;
> }
>+
>+ num_groups++;
> }
>
> *_out = out;
>- *_size = res->count;
>+ *_size = num_groups;
> }
>
Thank you, everything seems to work now. I'll just wait for the Coverity
and CI patches before pushing.
ACK.