URL: https://github.com/SSSD/sssd/pull/601 Author: pbrezina Title: #601: sbus: integrate sssd with sbus2 Action: opened
PR body: """ Hi folks, this is a really large patch set and I have now idea how to review it. I know that Jakub tried to review the proof of concept, but he have not managed to finish it so far. It is not in human power to review it all so hard testing should be done.
I consider the code itself and the integration with sssd finished. I will push only bug fixes and review issues now. This first version does not run through make check so far because some tests needs to be altered, this however does not prevent manual testing and reviewing.
I tried to make the changes small at first while running old and new sbus in parallel, but it was too hard as the changes were too much interconnected. Therefore each patch modifies one area, but completely and sssd will not build.
There are new libraries, each in pair. Each pair consist of asynchronous (used inside sssd) and synchronous (used inside tools; sssctl) versions of API. - libsss_sbus, libsss_sbus_sync: sbus interface - libsss_iface, libsss_iface_sync: internal sssd interface, used for internal communication - libifp_iface, libifp_iface_sync: infopipe interface
At this point, changes are mostly one to one. We still have separate server for monitor and backends. We still do not use signals, even though it is possible. I will file separate tickets with ideas how to improve our api further and we can work together after this patch set is tested and merged.
I did my best with manual testing but I doubt I run all the scenarious. Especially, I want to ask @sbose to test ifp smartcard API and @fidencio to test Fleet Commander integration (the one dbus call in ipa code).
Thank you. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/601/head:pr601 git checkout pr601
URL: https://github.com/SSSD/sssd/pull/601 Author: pbrezina Title: #601: sbus: integrate sssd with sbus2 Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/601/head:pr601 git checkout pr601
URL: https://github.com/SSSD/sssd/pull/601 Author: pbrezina Title: #601: sbus: integrate sssd with sbus2 Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/601/head:pr601 git checkout pr601
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ I pushed a new version. All unit tests are green now expect responder-get-domains-tests, which I can't get to work. There is a `wip fix responder-get-domains-tests` commit that I believe can fix the test, however it require to replace real function with its wrapper, i.e. to call `__wrap_sss_dp_get_domains_send` and `__wrap_sss_dp_get_domains_recv`. Unfortunately, the real function is still being called instead of the wrapper, do you guys have any idea why? """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-399073534
URL: https://github.com/SSSD/sssd/pull/601 Author: pbrezina Title: #601: sbus: integrate sssd with sbus2 Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/601/head:pr601 git checkout pr601
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ `make check`, `make intgcheck` and `make distcheck` pass locally. I disable the one problematic unit test for now. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-399751927
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ `make check`, `make intgcheck` and `make distcheck` pass locally. I disabled the one problematic unit test for now. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-399751927
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ CI pass now: http://vm-031.abc.idm.lab.eng.brq.redhat.com/logs/job/90/81/summary.html
There are two failure: * debian: not related to this patch set, it fails on other patches as well * rhel6: due to old version of libdbus, but this is ok since rhel6 will never see this code """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-400989347
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ Awesome. I will take a look once I'm back from my vacation.
About the RHEL-6 work, I think we might want to just drop RHEL-6 support upstream starting with the 2.x branch. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-401003169
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ Rebased on top of current master. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-407700563
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ Authentication on an IPA client doesn't work for me: ``` (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [ldb] (0x4000): commit ldb transaction (nesting: 0) (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [sysdb_set_entry_attr] (0x0200): Entry [name=admin@ipa.test,cn=users,cn=ipa.test,cn=sysdb] has set [cache, ts_cache] attrs. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [check_wait_queue] (0x1000): Wait queue for user [admin@ipa.test] is empty. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [krb5_auth_queue_done] (0x1000): krb5_auth_queue request [0xc25280] done. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [dp_req_done] (0x0400): DP Request [PAM Authenticate #7]: Request handler finished [0]: Success (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [_dp_req_recv] (0x0400): DP Request [PAM Authenticate #7]: Receiving request data. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [dp_req_destructor] (0x0400): DP Request [PAM Authenticate #7]: Request removed. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [dp_req_destructor] (0x0400): Number of active DP request: 0 (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [sbus_issue_request_done] (0x0400): sssd.dataprovider.pamHandler: Success (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [child_sig_handler] (0x1000): Waiting for child [21983]. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [child_sig_handler] (0x0100): child [21983] finished successfully. (Tue Jul 31 09:44:12 2018) [sssd[be[ipa.test]]] [sbus_dispatch] (0x4000): Dispatching. (Tue Jul 31 09:44:12 2018) [sssd[pam]] [sbus_dispatch] (0x4000): Dispatching. (Tue Jul 31 09:44:12 2018) [sssd[pam]] [sbus_iterator_read_pam_response] (0x0020): Unable to read pam data [1432158306]: Unexpected argument type provided (Tue Jul 31 09:44:12 2018) [sssd[pam]] [sbus_read_output] (0x0020): Unable to read message data [1432158306]: Unexpected argument type provided (Tue Jul 31 09:44:12 2018) [sssd[pam]] [pam_dp_send_req_done] (0x0020): PAM handler failed [1432158306]: Unexpected argument type provided ``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409161949
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ The old sbus implementation had quite a few tests that were generating some interface and then calling it. I also liked the tests because they were useful as a documentation. Do you plan on adding some unit tests for the new sbus library as well? """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409165823
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ It would also be nice to have some sort of a design page. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409166587
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ There is a large number of Coverity warnings that need to be fixed: ``` Error: CLANG_WARNING: sssd-1.16.3/src/providers/data_provider/dp_iface_backend.c:40:9: warning: Value stored to 'domain' is never read # domain = be_ctx->domain; # ^ ~~~~~~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_iface_backend.c:40:9: note: Value stored to 'domain' is never read # domain = be_ctx->domain; # ^ ~~~~~~~~~~~~~~ # 38| # 39| if (SBUS_REQ_STRING_IS_EMPTY(domname)) { # 40|-> domain = be_ctx->domain; # 41| *_is_online = be_is_offline(be_ctx); # 42| return EOK;
Error: UNINIT (CWE-457): sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:40: var_decl: Declaring variable "rules" without initializer. sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:85: uninit_use: Using uninitialized value "rules". # 83| *_dp_flags = dp_flags; # 84| *_sudo_type = sudo_type; # 85|-> *_rules = rules; # 86| # 87| return EOK;
Error: CLANG_WARNING: sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:85:13: warning: Assigned value is garbage or undefined # *_rules = rules; # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:125:9: note: Assuming 'req' is not equal to NULL # if (req == NULL) { # ^~~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:125:5: note: Taking false branch # if (req == NULL) { # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:131:9: note: Assuming the condition is false # if (state->data == NULL) { # ^~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:131:5: note: Taking false branch # if (state->data == NULL) { # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:136:11: note: Calling 'dp_sudo_parse_message' # ret = dp_sudo_parse_message(state, read_iter, &dp_flags, # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:40:5: note: 'rules' declared without an initial value # const char **rules; # ^~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:44:9: note: Assuming 'ret' is equal to EOK # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:44:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:50:9: note: Assuming 'ret' is equal to EOK # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:50:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:56:5: note: Control jumps to 'case 6:' at line 57 # switch (sudo_type) { # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:59:9: note: Execution continues on line 83 # break; # ^ sssd-1.16.3/src/providers/data_provider/dp_target_sudo.c:85:13: note: Assigned value is garbage or undefined # *_rules = rules; # ^ ~~~~~ # 83| *_dp_flags = dp_flags; # 84| *_sudo_type = sudo_type; # 85|-> *_rules = rules; # 86| # 87| return EOK;
Error: CLANG_WARNING: sssd-1.16.3/src/responder/ifp/ifp_groups.c:997:9: warning: Value stored to 'num_users' is never read # num_users = 0; # ^ ~ sssd-1.16.3/src/responder/ifp/ifp_groups.c:997:9: note: Value stored to 'num_users' is never read # num_users = 0; # ^ ~ # 995| users = NULL; # 996| groups = NULL; # 997|-> num_users = 0; # 998| num_groups = 0; # 999| ret = EOK;
Error: CLANG_WARNING: sssd-1.16.3/src/responder/ifp/ifp_groups.c:998:9: warning: Value stored to 'num_groups' is never read # num_groups = 0; # ^ ~ sssd-1.16.3/src/responder/ifp/ifp_groups.c:998:9: note: Value stored to 'num_groups' is never read # num_groups = 0; # ^ ~ # 996| groups = NULL; # 997| num_users = 0; # 998|-> num_groups = 0; # 999| ret = EOK; # 1000| goto done;
Error: FORWARD_NULL (CWE-476): sssd-1.16.3/src/sbus/codegen/sbus_Generator.py:762: assign_null: Assigning: "type" = "None". sssd-1.16.3/src/sbus/codegen/sbus_Generator.py:772: property_access: Accessing a property of null-like value "type". # 770| ) # 771| # 772|-> tpl.show("if-use-talloc", type.RequireTalloc) # 773| # 774| keys = {"input-signature": invoker.input.invokerSignature,
Error: UNREACHABLE (CWE-561): sssd-1.16.3/src/sbus/interface/sbus_introspection.c:252: unreachable: This code cannot be reached: "return "invalid";". # 250| } # 251| # 252|-> return "invalid"; # 253| } # 254|
Error: FORWARD_NULL (CWE-476): sssd-1.16.3/src/sbus/request/sbus_request.c:232: assign_zero: Assigning: "mainreq" = "NULL". sssd-1.16.3/src/sbus/request/sbus_request.c:260: var_deref_model: Passing null pointer "mainreq" to "sbus_requests_finish", which dereferences it. sssd-1.16.3/src/sbus/request/sbus_request_hash.c:268:5: deref_parm: Directly dereferencing parameter "item". # 266| errno_t error) # 267| { # 268|-> if (item->is_invalid) { # 269| return; # 270| }
Error: FORWARD_NULL (CWE-476): sssd-1.16.3/src/sbus/router/sbus_router_hash.c:352: var_compare_op: Comparing "a->object_path" to null implies that "a->object_path" might be null. sssd-1.16.3/src/sbus/router/sbus_router_hash.c:356: var_deref_model: Passing null pointer "a->object_path" to "strcmp", which dereferences it. # 354| } # 355| # 356|-> if (strcmp(a->object_path, b->object_path) != 0) { # 357| continue; # 358| }
Error: FORWARD_NULL (CWE-476): sssd-1.16.3/src/sbus/router/sbus_router_hash.c:348: var_compare_op: Comparing "b->object_path" to null implies that "b->object_path" might be null. sssd-1.16.3/src/sbus/router/sbus_router_hash.c:356: var_deref_model: Passing null pointer "b->object_path" to "strcmp", which dereferences it. # 354| } # 355| # 356|-> if (strcmp(a->object_path, b->object_path) != 0) { # 357| continue; # 358| }
Error: CLANG_WARNING: sssd-1.16.3/src/sbus/router/sbus_router_hash.c:356:13: warning: Null pointer passed as an argument to a 'nonnull' parameter # if (strcmp(a->object_path, b->object_path) != 0) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:427:9: note: Assuming 'tmp_ctx' is not equal to NULL # if (tmp_ctx == NULL) { # ^~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:427:5: note: Taking false branch # if (tmp_ctx == NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:433:9: note: Assuming 'key' is not equal to NULL # if (key == NULL) { # ^~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:433:5: note: Taking false branch # if (key == NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:439:9: note: Assuming 'item' is not equal to NULL # if (item == NULL) { # ^~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:439:5: note: Taking false branch # if (item == NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:444:22: note: Value assigned to field 'object_path' # item->listener = sbus_listener_copy(item, listener); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:445:9: note: Assuming the condition is false # if (item->listener == NULL) { # ^~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:445:5: note: Taking false branch # if (item->listener == NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:453:9: note: Assuming 'list' is not equal to NULL # if (list != NULL) { # ^~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:453:5: note: Taking true branch # if (list != NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:456:13: note: Calling 'sbus_listener_list_lookup' # if (sbus_listener_list_lookup(list, listener) != NULL) { # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:344:13: note: Assuming the condition is false # if (memcmp(&a->handler, &b->handler, sizeof(struct sbus_handler)) != 0) { # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:344:9: note: Taking false branch # if (memcmp(&a->handler, &b->handler, sizeof(struct sbus_handler)) != 0) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:348:13: note: Assuming pointer value is null # if (a->object_path == NULL && b->object_path != NULL) { # ^~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:348:13: note: Left side of '&&' is true sssd-1.16.3/src/sbus/router/sbus_router_hash.c:348:39: note: Assuming the condition is false # if (a->object_path == NULL && b->object_path != NULL) { # ^~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:348:9: note: Taking false branch # if (a->object_path == NULL && b->object_path != NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:352:36: note: Left side of '&&' is false # if (a->object_path != NULL && b->object_path == NULL) { # ^ sssd-1.16.3/src/sbus/router/sbus_router_hash.c:356:13: note: Null pointer passed as an argument to a 'nonnull' parameter # if (strcmp(a->object_path, b->object_path) != 0) { # ^ ~~~~~~~~~~~~~~ # 354| } # 355| # 356|-> if (strcmp(a->object_path, b->object_path) != 0) { # 357| continue; # 358| }
Error: CLANG_WARNING: sssd-1.16.3/src/sbus/server/sbus_server_match.c:335:9: warning: 1st function call argument is an uninitialized value # talloc_free(sbus_rule); # ^ /usr/include/talloc.h:228:26: note: expanded from macro 'talloc_free' ##define talloc_free(ctx) _talloc_free(ctx, __location__) # ^ sssd-1.16.3/src/sbus/server/sbus_server_match.c:375:11: note: Calling 'sbus_match_rule_parse' # ret = sbus_match_rule_parse(NULL, dbus_rule, &sbus_rule); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/server/sbus_server_match.c:308:5: note: 'sbus_rule' declared without an initial value # struct sbus_rule *sbus_rule; # ^~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/server/sbus_server_match.c:314:9: note: Assuming 'ret' is equal to EOK # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.16.3/src/sbus/server/sbus_server_match.c:314:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.16.3/src/sbus/server/sbus_server_match.c:318:11: note: Calling 'sbus_match_rule_parse_keys' # ret = sbus_match_rule_parse_keys(mem_ctx, tokens, &sbus_rule); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/server/sbus_server_match.c:253:9: note: Assuming 'rule' is equal to NULL # if (rule == NULL) { # ^~~~~~~~~~~~ sssd-1.16.3/src/sbus/server/sbus_server_match.c:253:5: note: Taking true branch # if (rule == NULL) { # ^ sssd-1.16.3/src/sbus/server/sbus_server_match.c:318:11: note: Returning from 'sbus_match_rule_parse_keys' # ret = sbus_match_rule_parse_keys(mem_ctx, tokens, &sbus_rule); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.3/src/sbus/server/sbus_server_match.c:320:5: note: Taking true branch # if (ret != EOK) { # ^ sssd-1.16.3/src/sbus/server/sbus_server_match.c:321:9: note: Control jumps to line 334 # goto done; # ^ sssd-1.16.3/src/sbus/server/sbus_server_match.c:334:5: note: Taking true branch # if (ret != EOK) { # ^ sssd-1.16.3/src/sbus/server/sbus_server_match.c:335:9: note: 1st function call argument is an uninitialized value # talloc_free(sbus_rule); # ^ ~~~~~~~~~ /usr/include/talloc.h:228:26: note: expanded from macro 'talloc_free' ##define talloc_free(ctx) _talloc_free(ctx, __location__) # ^ ~~~ # 333| done: # 334| if (ret != EOK) { # 335|-> talloc_free(sbus_rule); # 336| DEBUG(SSSDBG_OP_FAILURE, "Unable to parse rule [%s] [%d]: %s\n", # 337| dbus_rule, ret, sss_strerror(ret));
Error: CLANG_WARNING: sssd-1.16.3/src/sss_iface/sss_iface_types.c:378:5: warning: Value stored to 'resp' is never read # resp = pd->resp_list; # ^ ~~~~~~~~~~~~~ sssd-1.16.3/src/sss_iface/sss_iface_types.c:378:5: note: Value stored to 'resp' is never read # resp = pd->resp_list; # ^ ~~~~~~~~~~~~~ # 376| } # 377| # 378|-> resp = pd->resp_list; # 379| for (resp = pd->resp_list; resp != NULL; resp = resp->next) { # 380| dbret = dbus_message_iter_open_container(&array_iter, DBUS_TYPE_STRUCT, ``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409166916
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ Another test I tried: In one terminal, hammer sssd with requests in a for-loop: ``` for i in `seq 1 1000000`; do getent passwd foo$i; done ```
Then, in another terminall, kill sssd_be with `kill -9 $(pidof sssd_be)`. It looks like the responders never reconnect, all I could see was an endless string of `(Tue Jul 31 10:21:21 2018) [sssd[nss]] [sbus_dispatch] (0x0400): SBUS is reconnecting. Deferring`. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409172029
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """
The old sbus implementation had quite a few tests that were generating some interface and then calling it. I also liked the tests because they were useful as a documentation. Do you plan on adding some unit tests for the new sbus library as well?
I do plan to provide tests (and much broader set of tests) but it will not be done before the patches are merged as I need to focus elsewhere.
So far I fixed the coverity issues and squashed the changes in. Here is the diff: ```diff diff --git a/src/providers/data_provider/dp_iface_backend.c b/src/providers/data_provider/dp_iface_backend.c index 4066909..25a00f3 100644 --- a/src/providers/data_provider/dp_iface_backend.c +++ b/src/providers/data_provider/dp_iface_backend.c @@ -37,7 +37,6 @@ dp_backend_is_online(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain;
if (SBUS_REQ_STRING_IS_EMPTY(domname)) { - domain = be_ctx->domain; *_is_online = be_is_offline(be_ctx); return EOK; } diff --git a/src/providers/data_provider/dp_target_sudo.c b/src/providers/data_provider/dp_target_sudo.c index bc5f9c8..db14039 100644 --- a/src/providers/data_provider/dp_target_sudo.c +++ b/src/providers/data_provider/dp_target_sudo.c @@ -56,6 +56,7 @@ static errno_t dp_sudo_parse_message(TALLOC_CTX *mem_ctx, switch (sudo_type) { case BE_REQ_SUDO_FULL: /* no arguments required */ + rules = NULL; break; case BE_REQ_SUDO_RULES: /* additional arguments: diff --git a/src/responder/ifp/ifp_groups.c b/src/responder/ifp/ifp_groups.c index b6fae92..353f3a7 100644 --- a/src/responder/ifp/ifp_groups.c +++ b/src/responder/ifp/ifp_groups.c @@ -994,8 +994,6 @@ ifp_groups_group_get_members(TALLOC_CTX *mem_ctx, if (num_members == 0) { users = NULL; groups = NULL; - num_users = 0; - num_groups = 0; ret = EOK; goto done; } diff --git a/src/sbus/codegen/sbus_Generator.py b/src/sbus/codegen/sbus_Generator.py index 4e5e8bb..e950843 100644 --- a/src/sbus/codegen/sbus_Generator.py +++ b/src/sbus/codegen/sbus_Generator.py @@ -764,10 +764,14 @@ class Generator: type = DataType.Find( invoker.input.arguments['value'].signature ) - if self.hasAny(invoker.output): + elif self.hasAny(invoker.output): type = DataType.Find( invoker.output.arguments['value'].signature ) + else: + raise ValueError( + 'Invoker has no input nor output argument\n' + )
tpl.show("if-use-talloc", type.RequireTalloc)
diff --git a/src/sbus/interface/sbus_introspection.c b/src/sbus/interface/sbus_introspection.c index d4daedc..b2de9a9 100644 --- a/src/sbus/interface/sbus_introspection.c +++ b/src/sbus/interface/sbus_introspection.c @@ -248,8 +248,6 @@ sbus_introspect_property_mode(struct sbus_introspect_property *property) default: return "readwrite"; } - - return "invalid"; }
static errno_t diff --git a/src/sbus/request/sbus_request_hash.c b/src/sbus/request/sbus_request_hash.c index 68dc79c..441fce2 100644 --- a/src/sbus/request/sbus_request_hash.c +++ b/src/sbus/request/sbus_request_hash.c @@ -265,6 +265,11 @@ void sbus_requests_finish(struct sbus_request_list *item, errno_t error) { + if (item == NULL) { + DEBUG(SSSDBG_MINOR_FAILURE, "Bug: item is NULL\n"); + return; + } + if (item->is_invalid) { return; } diff --git a/src/sbus/router/sbus_router_hash.c b/src/sbus/router/sbus_router_hash.c index 41111bc..186dc61 100644 --- a/src/sbus/router/sbus_router_hash.c +++ b/src/sbus/router/sbus_router_hash.c @@ -337,7 +337,8 @@ sbus_listener_list_lookup(struct sbus_listener_list *list, struct sbus_listener_list *item; struct sbus_listener *b;
- /* We know that interface and signal name already match. */ + /* We know that interface and signal name already match. We need to check + * handlers and object paths. */ DLIST_FOR_EACH(item, list) { b = item->listener;
@@ -345,6 +346,10 @@ sbus_listener_list_lookup(struct sbus_listener_list *list, continue; }
+ if (a->object_path == NULL && b->object_path == NULL) { + return b; + } + if (a->object_path == NULL && b->object_path != NULL) { continue; } diff --git a/src/sbus/server/sbus_server_match.c b/src/sbus/server/sbus_server_match.c index 6a65328..07bdf5b 100644 --- a/src/sbus/server/sbus_server_match.c +++ b/src/sbus/server/sbus_server_match.c @@ -312,7 +312,7 @@ sbus_match_rule_parse(TALLOC_CTX *mem_ctx,
ret = split_on_separator(NULL, dbus_rule, ',', true, true, &tokens, &count); if (ret != EOK) { - return ret; + goto done; }
ret = sbus_match_rule_parse_keys(mem_ctx, tokens, &sbus_rule); @@ -323,6 +323,7 @@ sbus_match_rule_parse(TALLOC_CTX *mem_ctx,
ret = sbus_match_rule_parse_check(sbus_rule); if (ret != EOK) { + talloc_free(sbus_rule); goto done; }
@@ -332,9 +333,8 @@ sbus_match_rule_parse(TALLOC_CTX *mem_ctx,
done: if (ret != EOK) { - talloc_free(sbus_rule); DEBUG(SSSDBG_OP_FAILURE, "Unable to parse rule [%s] [%d]: %s\n", - dbus_rule, ret, sss_strerror(ret)); + dbus_rule, ret, sss_strerror(ret)); }
diff --git a/src/sss_iface/sss_iface_types.c b/src/sss_iface/sss_iface_types.c index bb4fd02..3c0bbc0 100644 --- a/src/sss_iface/sss_iface_types.c +++ b/src/sss_iface/sss_iface_types.c @@ -375,7 +375,6 @@ errno_t sbus_iterator_write_pam_response(DBusMessageIter *iterator, goto done; }
- resp = pd->resp_list; for (resp = pd->resp_list; resp != NULL; resp = resp->next) { dbret = dbus_message_iter_open_container(&array_iter, DBUS_TYPE_STRUCT, NULL, &struct_iter);
``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409511174
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ Fixed IPA issue:
```diff diff --git a/src/providers/data_provider/dp_target_auth.c b/src/providers/data_provider/dp_target_auth.c index 13874f9..02179dd 100644 --- a/src/providers/data_provider/dp_target_auth.c +++ b/src/providers/data_provider/dp_target_auth.c @@ -222,16 +222,16 @@ static void dp_pam_handler_auth_done(struct tevent_req *subreq) return; }
- req = dp_req_send(state, state->provider, state->pd->domain, - "PAM SELinux", DPT_SELINUX, DPM_SELINUX_HANDLER, - 0, state->pd, NULL); - if (req == NULL) { + subreq = dp_req_send(state, state->provider, state->pd->domain, + "PAM SELinux", DPT_SELINUX, DPM_SELINUX_HANDLER, + 0, state->pd, NULL); + if (subreq == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create subrequest!\n"); tevent_req_error(req, ENOMEM); return; }
- tevent_req_set_callback(req, dp_pam_handler_done, req); + tevent_req_set_callback(subreq, dp_pam_handler_done, req); }
static void dp_pam_handler_done(struct tevent_req *subreq) diff --git a/src/sss_iface/sss_iface_types.c b/src/sss_iface/sss_iface_types.c index 3c0bbc0..b2b3098 100644 --- a/src/sss_iface/sss_iface_types.c +++ b/src/sss_iface/sss_iface_types.c @@ -311,7 +311,7 @@ errno_t sbus_iterator_read_pam_response(TALLOC_CTX *mem_ctx,
dbus_message_iter_recurse(&array_iter, &struct_iter);
- ret = sbus_iterator_read_u(iterator, &resp_type); + ret = sbus_iterator_read_u(&struct_iter, &resp_type); if (ret != EOK) { goto done; } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409562828
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ Reconnection issue is fixed so everything @jhrozek raised is solved. Diff:
```diff diff --git a/src/sbus/connection/sbus_connection_connect.c b/src/sbus/connection/sbus_connection_connect.c index bbe8a4c..3f8702f 100644 --- a/src/sbus/connection/sbus_connection_connect.c +++ b/src/sbus/connection/sbus_connection_connect.c @@ -40,8 +40,8 @@ static void sbus_connect_init_done(struct tevent_req *subreq);
struct tevent_req * sbus_connect_init_send(TALLOC_CTX *mem_ctx, - struct sbus_connection *conn, - const char *name) + struct sbus_connection *conn, + const char *name) { struct sbus_connect_init_state *state; struct tevent_req *subreq; diff --git a/src/sbus/connection/sbus_reconnect.c b/src/sbus/connection/sbus_reconnect.c index 9c5e560..a0055e5 100644 --- a/src/sbus/connection/sbus_reconnect.c +++ b/src/sbus/connection/sbus_reconnect.c @@ -60,8 +60,6 @@ sbus_reconnect_success(struct sbus_connection *conn) sbus_reconnect_notify(conn, SBUS_RECONNECT_SUCCESS); }
-static void sbus_reconnect_attempt_done(struct tevent_req *req); - static void sbus_reconnect_attempt(struct tevent_context *ev, struct tevent_timer *te, @@ -70,8 +68,6 @@ sbus_reconnect_attempt(struct tevent_context *ev, { struct sbus_connection *sbus_conn; DBusConnection *dbus_conn = NULL; - bool init_connection = false; - struct tevent_req *req; errno_t ret;
sbus_conn = talloc_get_type(data, struct sbus_connection); @@ -90,10 +86,13 @@ sbus_reconnect_attempt(struct tevent_context *ev, case SBUS_CONNECTION_ADDRESS: DEBUG(SSSDBG_MINOR_FAILURE, "Making reconnection attempt %d to [%s]\n", sbus_conn->reconnect->retry.current, sbus_conn->address); - init_connection = true; + /* It is necessary to use blocking Hello and RequestName method + * so those two are the only methods that are sent to the new + * dbus connection before it is properly initialized. + */ dbus_conn = sbus_dbus_connect_address(sbus_conn->address, sbus_conn->wellknown_name, - false); + true); break; case SBUS_CONNECTION_SYSBUS: DEBUG(SSSDBG_MINOR_FAILURE, "Making reconnection attempt %d " @@ -117,18 +116,7 @@ sbus_reconnect_attempt(struct tevent_context *ev, goto done; }
- if (!init_connection) { - goto done; - } - - req = sbus_connect_init_send(sbus_conn, sbus_conn, sbus_conn->wellknown_name); - if (req == NULL) { - ret = ENOMEM; - goto done; - } - - tevent_req_set_callback(req, sbus_reconnect_attempt_done, sbus_conn); - return; + ret = EOK;
done: /* Issue next attempt or finish. */ @@ -140,24 +128,6 @@ done: sbus_reconnect_success(sbus_conn); }
-static void sbus_reconnect_attempt_done(struct tevent_req *req) -{ - struct sbus_connection *conn; - errno_t ret; - - conn = tevent_req_callback_data(req, struct sbus_connection); - - ret = sbus_connect_init_recv(req); - talloc_zfree(req); - /* Issue next attempt or finish. */ - if (ret != EOK) { - sbus_reconnect(conn); - return; - } - - sbus_reconnect_success(conn); -} - static struct timeval sbus_reconnect_delay(struct sbus_reconnect *reconnect) { @@ -211,7 +181,7 @@ void sbus_reconnect(struct sbus_connection *conn) /* Increase retry counter and check if we are still allowed to reconnect. */ reconnect->retry.current++;
- if (reconnect->retry.current >= reconnect->retry.max) { + if (reconnect->retry.current > reconnect->retry.max) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to reconnect: maximum retries exceeded.\n"); sbus_reconnect_notify(conn, SBUS_RECONNECT_EXCEEDED_RETRIES); diff --git a/src/sbus/server/sbus_server_interface.c b/src/sbus/server/sbus_server_interface.c index 209532a..695d4d0 100644 --- a/src/sbus/server/sbus_server_interface.c +++ b/src/sbus/server/sbus_server_interface.c @@ -108,6 +108,8 @@ sbus_server_bus_request_name(TALLOC_CTX *mem_ctx, struct sbus_connection *conn; errno_t ret;
+ DEBUG(SSSDBG_TRACE_FUNC, "Requesting name: %s\n", name); + if (name[0] == ':') { DEBUG(SSSDBG_OP_FAILURE, "Can not assign unique name: %s\n", name); return EINVAL; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-409583236
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: +postponed until sssd 2.0
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ Another issue revelaed by tests is that starting with `user=sssd` does not work at the moment.. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412037717
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ Right. The problem was when reading the keytab as you wrote in the mail. I did not notice it because my test keytab is readable by all.
I moved `become_user` after the point where data provider is fully initialized so the initialization is done as root as it was before.
```diff +diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 095966b25..670ddb477 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -582,6 +582,23 @@ static void dp_initialized(struct tevent_req *req) goto done; }
+ ret = chown_debug_file(NULL, be_ctx->uid, be_ctx->gid); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Cannot chown the debug files, debugging might not work!\n"); + } + + ret = become_user(be_ctx->uid, be_ctx->gid); + if (ret != EOK) { + DEBUG(SSSDBG_FUNC_DATA, + "Cannot become user [%"SPRIuid"][%"SPRIgid"].\n", + be_ctx->uid, be_ctx->gid); + goto done; + } + + DEBUG(SSSDBG_TRACE_FUNC, "Backend provider (%s) started!\n", + be_ctx->domain->name); + ret = EOK;
done: @@ -678,21 +695,6 @@ int main(int argc, const char *argv[]) return 3; }
- ret = chown_debug_file(NULL, uid, gid); - if (ret != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, - "Cannot chown the debug files, debugging might not work!\n"); - } - - ret = become_user(uid, gid); - if (ret != EOK) { - DEBUG(SSSDBG_FUNC_DATA, - "Cannot become user [%"SPRIuid"][%"SPRIgid"].\n", uid, gid); - return ret; - } - - DEBUG(SSSDBG_TRACE_FUNC, "Backend provider (%s) started!\n", be_domain); - /* loop on main */ server_loop(main_ctx); ``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412063892
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ There are still some Coverity warnings: ``` Error: CLANG_WARNING: sssd-1.16.4/src/monitor/monitor.c:2138:9: warning: Value stored to 'ret' is never read # ret = notify_startup(); # ^ ~~~~~~~~~~~~~~~~ sssd-1.16.4/src/monitor/monitor.c:2138:9: note: Value stored to 'ret' is never read # ret = notify_startup(); # ^ ~~~~~~~~~~~~~~~~ # 2136| * timeout! */ # 2137| if (num_providers == 0 && ctx->services == NULL) { # 2138|-> ret = notify_startup(); # 2139| } # 2140|
Error: CPPCHECK_WARNING (CWE-456): sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:271: error[uninitvar]: Uninitialized variable: pd # 269| } # 270| # 271|-> if (!should_invoke_selinux(state->provider, pd)) { # 272| /* State and request related data are freed with sbus_req. */ # 273| dp_pam_reply(state->sbus_req, state->request_name, pd);
Error: UNINIT (CWE-457): sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:259: var_decl: Declaring variable "pd" without initializer. sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:271: uninit_use_in_call: Using uninitialized value "pd" when calling "should_invoke_selinux". sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:249:5: read_parm: Reading a parameter value. # 269| } # 270| # 271|-> if (!should_invoke_selinux(state->provider, pd)) { # 272| /* State and request related data are freed with sbus_req. */ # 273| dp_pam_reply(state->sbus_req, state->request_name, pd);
Error: UNINIT (CWE-457): sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:259: var_decl: Declaring variable "pd" without initializer. sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:273: uninit_use_in_call: Using uninitialized value "pd" when calling "dp_pam_reply". sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:49:5: read_parm: Reading a parameter value. # 271| if (!should_invoke_selinux(state->provider, pd)) { # 272| /* State and request related data are freed with sbus_req. */ # 273|-> dp_pam_reply(state->sbus_req, state->request_name, pd); # 274| return; # 275| }
Error: CPPCHECK_WARNING (CWE-456): sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:306: error[uninitvar]: Uninitialized variable: pd # 304| # 305| /* State and request related data are freed with sbus_req. */ # 306|-> dp_pam_reply(state->sbus_req, state->request_name, pd); # 307| return; # 308| }
Error: UNINIT (CWE-457): sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:293: var_decl: Declaring variable "pd" without initializer. sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:306: uninit_use_in_call: Using uninitialized value "pd" when calling "dp_pam_reply". sssd-1.16.4/src/providers/data_provider/dp_target_auth.c:49:5: read_parm: Reading a parameter value. # 304| # 305| /* State and request related data are freed with sbus_req. */ # 306|-> dp_pam_reply(state->sbus_req, state->request_name, pd); # 307| return; # 308| }
Error: CLANG_WARNING: sssd-1.16.4/src/responder/ifp/ifp_cache.c:273:5: warning: 2nd function call argument is an uninitialized value # iface_ifp_cache_ListByDomain_finish(sbus_req, paths, num_paths); # ^ ~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:249:5: note: 'paths' declared without an initial value # const char **paths; # ^~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:254:9: note: Assuming 'ifp_ctx' is not equal to NULL # if (ifp_ctx == NULL) { # ^~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:254:5: note: Taking false branch # if (ifp_ctx == NULL) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:260:9: note: Assuming 'domain' is not equal to NULL # if (domain == NULL) { # ^~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:260:5: note: Taking false branch # if (domain == NULL) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:265:11: note: Calling 'ifp_cache_get_cached_objects' # ret = ifp_cache_get_cached_objects(sbus_req, type, domain, # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:98:25: note: Calling 'ifp_cache_object_class' # const char *class = ifp_cache_object_class(type); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:76:5: note: Control jumps to 'case IFP_CACHE_GROUP:' at line 80 # switch (type) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:82:9: note: Execution continues on line 85 # break; # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:98:25: note: Returning from 'ifp_cache_object_class' # const char *class = ifp_cache_object_class(type); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:107:9: note: Assuming 'tmp_ctx' is not equal to NULL # if (tmp_ctx == NULL) { # ^~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:107:5: note: Taking false branch # if (tmp_ctx == NULL) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:111:15: note: Calling 'ifp_cache_build_base_dn' # base_dn = ifp_cache_build_base_dn(tmp_ctx, type, domain); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:39:5: note: Control jumps to 'case IFP_CACHE_GROUP:' at line 43 # switch (type) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:45:9: note: Execution continues on line 48 # break; # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:111:15: note: Returning from 'ifp_cache_build_base_dn' # base_dn = ifp_cache_build_base_dn(tmp_ctx, type, domain); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:112:9: note: Assuming 'base_dn' is not equal to NULL # if (base_dn == NULL) { # ^~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:112:5: note: Taking false branch # if (base_dn == NULL) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:122:9: note: Assuming 'ldb_ret' is not equal to LDB_SUCCESS # if (ldb_ret != LDB_SUCCESS) { # ^~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:122:5: note: Taking true branch # if (ldb_ret != LDB_SUCCESS) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:123:9: note: Assuming the condition is false # DEBUG(SSSDBG_CRIT_FAILURE, "Unable to search the cache\n"); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET' ##define DEBUG_IS_SET(level) (debug_level & (level) || \ # ^~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:123:9: note: Left side of '||' is false sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^ sssd-1.16.4/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET' ##define DEBUG_IS_SET(level) (debug_level & (level) || \ # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:123:9: note: Assuming 'debug_level' is not equal to DEBUG # DEBUG(SSSDBG_CRIT_FAILURE, "Unable to search the cache\n"); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:136:30: note: expanded from macro 'DEBUG_IS_SET' # (debug_level == SSSDBG_UNRESOLVED && \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:123:9: note: Left side of '&&' is false sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^ sssd-1.16.4/src/util/debug.h:136:63: note: expanded from macro 'DEBUG_IS_SET' # (debug_level == SSSDBG_UNRESOLVED && \ # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:123:9: note: Loop condition is false. Exiting loop sssd-1.16.4/src/util/debug.h:121:35: note: expanded from macro 'DEBUG' ##define DEBUG(level, format, ...) do { \ # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:125:9: note: Control jumps to line 148 # goto done; # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:265:11: note: Returning from 'ifp_cache_get_cached_objects' # ret = ifp_cache_get_cached_objects(sbus_req, type, domain, # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:267:9: note: Assuming 'ret' is equal to EOK # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.16.4/src/responder/ifp/ifp_cache.c:267:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.16.4/src/responder/ifp/ifp_cache.c:273:5: note: 2nd function call argument is an uninitialized value # iface_ifp_cache_ListByDomain_finish(sbus_req, paths, num_paths); # ^ ~~~~~ # 271| } # 272| # 273|-> iface_ifp_cache_ListByDomain_finish(sbus_req, paths, num_paths); # 274| # 275| return EOK;
Error: CLANG_WARNING: sssd-1.16.4/src/sbus/sssd_dbus_connection.c:290:21: warning: 1st function call argument is an uninitialized value # conn->address = talloc_strdup(conn, address); # ^ ~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:266:5: note: 'conn' declared without an initial value # struct sbus_connection *conn; # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:275:9: note: Assuming 'dbus_conn' is non-null # if (!dbus_conn) { # ^~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:275:5: note: Taking false branch # if (!dbus_conn) { # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:283:11: note: Calling 'sbus_init_connection' # ret = sbus_init_connection(ctx, ev, dbus_conn, SBUS_CONN_TYPE_SHARED, # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:156:5: note: Assuming the condition is false # DEBUG(SSSDBG_TRACE_FUNC,"Adding connection %p\n", dbus_conn); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET' ##define DEBUG_IS_SET(level) (debug_level & (level) || \ # ^~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:156:5: note: Left side of '||' is false sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^ sssd-1.16.4/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET' ##define DEBUG_IS_SET(level) (debug_level & (level) || \ # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:156:5: note: Assuming 'debug_level' is not equal to DEBUG # DEBUG(SSSDBG_TRACE_FUNC,"Adding connection %p\n", dbus_conn); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:136:30: note: expanded from macro 'DEBUG_IS_SET' # (debug_level == SSSDBG_UNRESOLVED && \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:156:5: note: Left side of '&&' is false sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^ sssd-1.16.4/src/util/debug.h:136:63: note: expanded from macro 'DEBUG_IS_SET' # (debug_level == SSSDBG_UNRESOLVED && \ # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:156:5: note: Loop condition is false. Exiting loop sssd-1.16.4/src/util/debug.h:121:35: note: expanded from macro 'DEBUG' ##define DEBUG(level, format, ...) do { \ # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:167:9: note: Assuming the condition is true # if (conn->managed_paths == NULL) { # ^~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:167:5: note: Taking true branch # if (conn->managed_paths == NULL) { # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:168:9: note: Assuming the condition is false # DEBUG(SSSDBG_CRIT_FAILURE, "Cannot create object paths hash table\n"); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET' ##define DEBUG_IS_SET(level) (debug_level & (level) || \ # ^~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:168:9: note: Left side of '||' is false sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^ sssd-1.16.4/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET' ##define DEBUG_IS_SET(level) (debug_level & (level) || \ # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:168:9: note: Assuming 'debug_level' is not equal to DEBUG # DEBUG(SSSDBG_CRIT_FAILURE, "Cannot create object paths hash table\n"); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/util/debug.h:136:30: note: expanded from macro 'DEBUG_IS_SET' # (debug_level == SSSDBG_UNRESOLVED && \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:168:9: note: Left side of '&&' is false sssd-1.16.4/src/util/debug.h:123:9: note: expanded from macro 'DEBUG' # if (DEBUG_IS_SET(__debug_macro_level)) { \ # ^ sssd-1.16.4/src/util/debug.h:136:63: note: expanded from macro 'DEBUG_IS_SET' # (debug_level == SSSDBG_UNRESOLVED && \ # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:168:9: note: Loop condition is false. Exiting loop sssd-1.16.4/src/util/debug.h:121:35: note: expanded from macro 'DEBUG' ##define DEBUG(level, format, ...) do { \ # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:283:11: note: Returning from 'sbus_init_connection' # ret = sbus_init_connection(ctx, ev, dbus_conn, SBUS_CONN_TYPE_SHARED, # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:285:5: note: Taking true branch # if (ret != EOK) { # ^ sssd-1.16.4/src/sbus/sssd_dbus_connection.c:290:21: note: 1st function call argument is an uninitialized value # conn->address = talloc_strdup(conn, address); # ^ ~~~~ # 288| # 289| /* Store the address for later reconnection */ # 290|-> conn->address = talloc_strdup(conn, address); # 291| # 292| dbus_connection_set_exit_on_disconnect(conn->dbus.conn, FALSE);
Error: CLANG_WARNING: sssd-1.16.4/src/sbus/sssd_dbus_request.c:322:13: warning: Value stored to 'arg_ptr' is never read # arg_ptr = va_arg(va, void**); # ^ ~~~~~~~~~~~~~~~~~~ sssd-1.16.4/src/sbus/sssd_dbus_request.c:322:13: note: Value stored to 'arg_ptr' is never read # arg_ptr = va_arg(va, void**); # ^ ~~~~~~~~~~~~~~~~~~ # 320| /* A non array argument */ # 321| } else { # 322|-> arg_ptr = va_arg(va, void**); # 323| } # 324|
Error: TOCTOU (CWE-367): sssd-1.16.4/src/sbus/sssd_dbus_server.c:141: fs_check_call: Calling function "readlink" to perform check on "symlink_name". sssd-1.16.4/src/sbus/sssd_dbus_server.c:171: toctou: Calling function "unlink" that uses "symlink_name" after a check function. This can cause a time-of-check, time-of-use race condition. # 169| } # 170| # 171|-> ret = unlink(symlink_name); # 172| if (ret != 0) { # 173| ret = errno; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412369641
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ No, sorry, please ignore. Coverity was telling me that these were "fixed", so these are false positives from the earlier code. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412370536
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ I've started some more beaker test to be sure everything is OK before acking.. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412370563
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ Thank you. Please also do not forget the get domains test issue: https://github.com/SSSD/sssd/pull/601#issuecomment-399073534 I still was not able to figure this one out. But if necessary, we can disable this test and create a ticket for fixing it. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412443003
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ I can't figure that one either. Please create a ticket into 2.1 and resubmit the patches with the test disabled.
btw the beaker test with the non-root user is still failing, but manually everything works OK now. So I think once you resubmit with a nicer patch to disable the test, we can push the patches.. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412445126
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ And one more thing - after I ran make, I seem to have a newline in each of the generated files, e.g.: ``` diff --git a/src/responder/ifp/ifp_iface/sbus_ifp_arguments.c b/src/responder/ifp/ifp_iface/sbus_ifp_arguments.c index 031db83ca..caf862d45 100644 --- a/src/responder/ifp/ifp_iface/sbus_ifp_arguments.c +++ b/src/responder/ifp/ifp_iface/sbus_ifp_arguments.c @@ -395,3 +395,4 @@ errno_t _sbus_ifp_invoker_write_u
return EOK; } + diff --git a/src/responder/ifp/ifp_iface/sbus_ifp_client_async.c b/src/responder/ifp/ifp_iface/sbus_ifp_client_async.c index 792061c62..d251e016c 100644 --- a/src/responder/ifp/ifp_iface/sbus_ifp_client_async.c +++ b/src/responder/ifp/ifp_iface/sbus_ifp_client_async.c @@ -28,3 +28,4 @@ #include "responder/ifp/ifp_iface/sbus_ifp_arguments.h" #include "responder/ifp/ifp_iface/sbus_ifp_keygens.h" #include "responder/ifp/ifp_iface/sbus_ifp_client_properties.h" + ```
Did something change in the codegen that needs to be commited? """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412448997
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
pbrezina commented: """ Ticket to fix tests: https://pagure.io/SSSD/sssd/issue/3806
I included new patch to disable failing tests and fixed the whitespace issue. It turned out there were few trailing whitespaces in codegen templates that were removed by Jakub's `git am` and thus the files got regenerated. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412469626
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ I think this PR has gotten sufficient testing given its sheer size. """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412499930
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
jhrozek commented: """ * master: c0c8499b6ac36b44e3db7e8c1d44ce32f38662b5 3d1b6458568f3df4d5c192f432e73d65e4a9d293 7c1dd71c31faa00039f764af45a28e8668731769 5edba6ce412f5be57a36fac1a7bd2b884c36ddf3 aaecabf2d0e7a0732e4169b9b7c09a266603abd8 fbe2476a3dd9be83ffa85c29dca26f734618d72d de3a63c4b8e55ae2dbd3fbf12ca868c854ed8052 e50fb8ace7e1af34bd0742a3683343e5f0055ae9 c7e2d7a56d8969f26921811e5122f78b257fb51e 924f8098333e07d7bc2675f93cf4e57ec489a194 2963f2d917bd904f72b11daf400293ba518c74a0 06631b456011c630029f6fd2d1c3e97d95bab710 f91e90a7690e928e127a6f1020bb125c79377992 7f3ed078768d39153ef7929bd97fae047cf18965 b49ee1bfc3b541b08d7c7b7188d8bde3dbd1bd44 564c0798adbcfe132509d5245ea5b7fa2e4d3b51 e347b55571e1f08b48865e06a86d2f009442e10a 9c9a432833476053c46859ae67b0cfa19a41efa2 c2ed0caee434accef1999e5794daa12322c73bb7 40e3863ef226eac58d74ea7fcb25dcfa99672dc5 7e9f0a0c99bc8eace31f44c4c5e9ce7e13d76680 """
See the full comment at https://github.com/SSSD/sssd/pull/601#issuecomment-412549579
URL: https://github.com/SSSD/sssd/pull/601 Title: #601: sbus: integrate sssd with sbus2
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/601 Author: pbrezina Title: #601: sbus: integrate sssd with sbus2 Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/601/head:pr601 git checkout pr601
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
pbrezina