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;
}