[SSSD] [PRELIMINARY][PATCH] ifp users and groups

Jakub Hrozek jhrozek at redhat.com
Fri May 22 11:43:23 UTC 2015


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 at 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 at 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 at 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 at 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 at 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.



More information about the sssd-devel mailing list