URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: opened
PR body: """ Given the size of the current nss responder it was quite impossible to simply switch into using the cache_req interface, especially because most of the code was duplication of cache lookups.
This patch completely rewrites the responder from scratch. The amount of code was reduced to less than a half lines of code with no code duplication, better documentation and better maintainability and readability.
All functionality should be intact.
*Code organization* All protocol (parsing input message and send a reply) is placed in nss_protocol.c. Functions that deals with creating a reply packet are placed into their specific nss_protocol_$object.c files.
All supported commands are placed into nss_cmd.c. Functions that deals with cache req are in nss_get_object.c and nss_enum.c.
*Code flow for non-enumeration* An nss_getby_$input-type is called for each non-enumeration command. This function parses the input message, creates a cache_req_data structure and issues nss_get_object that calls cache_req. When this request is done nss_getby_done make sure a reply is sent to the client.
*Comments on enumeration* I made some effort to make sure enumeration shares the same code for users, groups, services and netgroups. Netgroups now uses nss negative cache instead of implementing its own.
*Unit tests* ALl existing unit tests, including the one for nss responder, pass. @mzidek-rh is going to write new unit tests for added functionality into cache_req and sss_ptr_hash interface.
Thanks to @spbnick for doing the first round of review, focused on code documentation and readibility. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: edited
Changed field: body Original value: """ Given the size of the current nss responder it was quite impossible to simply switch into using the cache_req interface, especially because most of the code was duplication of cache lookups.
This patch completely rewrites the responder from scratch. The amount of code was reduced to less than a half lines of code with no code duplication, better documentation and better maintainability and readability.
All functionality should be intact.
*Code organization* All protocol (parsing input message and send a reply) is placed in nss_protocol.c. Functions that deals with creating a reply packet are placed into their specific nss_protocol_$object.c files.
All supported commands are placed into nss_cmd.c. Functions that deals with cache req are in nss_get_object.c and nss_enum.c.
*Code flow for non-enumeration* An nss_getby_$input-type is called for each non-enumeration command. This function parses the input message, creates a cache_req_data structure and issues nss_get_object that calls cache_req. When this request is done nss_getby_done make sure a reply is sent to the client.
*Comments on enumeration* I made some effort to make sure enumeration shares the same code for users, groups, services and netgroups. Netgroups now uses nss negative cache instead of implementing its own.
*Unit tests* ALl existing unit tests, including the one for nss responder, pass. @mzidek-rh is going to write new unit tests for added functionality into cache_req and sss_ptr_hash interface.
Thanks to @spbnick for doing the first round of review, focused on code documentation and readibility. """
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Congratulation you broke integration tests on all distributions. ``` ldap_local_override_test.py::test_regr_2790_override FAILED test_memory_cache.py::test_getgrnam_simple_with_mc FAILED test_memory_cache.py::test_getgrnam_membership_with_mc FAILED test_memory_cache.py::test_invalidation_of_gids_after_initgroups FAILED test_memory_cache.py::test_initgroups_without_change_in_membership FAILED test_netgroup.py::test_remove_step_by_step FAILED test_netgroup.py::test_removing_nested_netgroups FAILED test_netgroup.py::test_offline_netgroups FAILED ```
and uninitialized variable is used. ``` src/responder/nss/nss_protocol_svcent.c: In function ‘nss_protocol_fill_svcent’: src/responder/nss/nss_protocol_svcent.c:134:15: error: ‘num_aliases’ may be used uninitialized in this function [-Werror=maybe-uninitialized] aliases = talloc_zero_array(mem_ctx, struct sized_string, num_aliases + 1); ^~~~~~~~~~~~~~~~~ src/responder/nss/nss_protocol_svcent.c:121:14: note: ‘num_aliases’ was declared here uint32_t num_aliases; ^~~~~~~~~~~ CC src/responder/nss/nss_iface.o ``` Maybe it's related to failures in integration tests. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-262825019
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ and other warnings from static analyzers ``` Error: NULL_RETURNS (CWE-476): [#def42] sssd-1.14.90/src/util/sss_ptr_hash.c:302: returned_null: "sss_ptr_hash_lookup_internal" returns null. sssd-1.14.90/src/util/sss_ptr_hash.c:251:9: return_null: Explicitly returning null. sssd-1.14.90/src/util/sss_ptr_hash.c:302: var_assigned: Assigning: "value" = null return value from "sss_ptr_hash_lookup_internal". sssd-1.14.90/src/util/sss_ptr_hash.c:303: dereference: Dereferencing a null pointer "value". # 301| # 302| value = sss_ptr_hash_lookup_internal(table, key); # 303|-> ptr = value->ptr; # 304| # 305| table_key.type = HASH_KEY_STRING;
Error: UNUSED_VALUE (CWE-563): [#def43] sssd-1.14.90/src/util/usertools.c:794: assigned_value: Assigning value "12" to "ret" here, but that stored value is not used. # 792| shortname = talloc_strdup(tmp_ctx, name); # 793| if (shortname == NULL) { # 794|-> ret = ENOMEM; # 795| goto done; # 796| } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-262905530
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ The 1st patch broke `nss-srv-tests` and therefore all following commits does not pass CI. ``` Subject: nss: move nss_ctx->global_names to rctx Hash: c64615d84e7c9dfdcb26762596a25937578ee94d
==31198== Invalid write of size 8 ==31198== at 0x6A29643: sss_names_init_from_args (usertools.c:198) ==31198== by 0x1228A0: test_nss_setup (test_nss_srv.c:1236) ==31198== by 0x122B67: nss_test_setup (test_nss_srv.c:3079) ==31198== by 0x4E3B6F1: ??? (in /usr/lib/x86_64-linux-gnu/libcmocka.so.0.3.1) ==31198== by 0x4E3BD39: _cmocka_run_group_tests (in /usr/lib/x86_64-linux-gnu/libcmocka.so.0.3.1) ==31198== by 0x11131C: main (test_nss_srv.c:3754) ==31198== Address 0x48 is not stack'd, malloc'd or (recently) free'd ==31198== { <insert_a_suppression_name_here> Memcheck:Addr8 fun:sss_names_init_from_args fun:test_nss_setup fun:nss_test_setup obj:/usr/lib/x86_64-linux-gnu/libcmocka.so.0.3.1 fun:_cmocka_run_group_tests fun:main } ```
http://sssd-ci.duckdns.org/logs/job/57/47/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-262907840
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
spbnick commented: """ In general it is good to submit your patchset to CI with the tag "each", to have each patch tested (instead of only the last one), and catch this kind of thing. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-262908628
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ Hi, thank you. I fixed those issues. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-262944217
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ There is a Wformat-security warning and therefore mock build failed on fedora ``` src/db/sysdb_ops.c: In function ‘sysdb_search_object_attr’: src/db/sysdb_ops.c:4503:22: error: format not a string literal and no format arguments [-Werror=format-security] LDB_SCOPE_SUBTREE, attrs?attrs:def_attrs, filter); ^~~~~~~~~~~~~~~~~ ```
The solution is quite simple ``` diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 4150c33..e8ecdeb 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -4500,7 +4500,7 @@ static errno_t sysdb_search_object_attr(TALLOC_CTX *mem_ctx, }
ret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res, basedn, - LDB_SCOPE_SUBTREE, attrs?attrs:def_attrs, filter); + LDB_SCOPE_SUBTREE, attrs?attrs:def_attrs, "%s", filter); if (ret != EOK) { ret = sysdb_error_to_errno(ret); DEBUG(SSSDBG_OP_FAILURE, "ldb_search failed.\n"); ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-263633818
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Regression test for ticket https://fedorahosted.org/sssd/ticket/1229 failed. ```
/var/log/sssd/sssd_nss.log"
// clear case and restart sssd kill -STOP `pidof sssd_be`" sleep 10" getent -s sss passwd usr1 &" kill -9 `pidof sssd_be`" sleep 10" getent -s sss passwd usr1"
AssertNotGrep "Identical request in progress" "/var/log/sssd/sssd_nss.log" This assetion failed. ```
I am not sure how reliable is this test but:
- it passes with master - you didn't remove related debug message
sssd.conf was really trivial ``` [domain/LDAP] debug_level=0xFFF0 id_provider = ldap ldap_uri = ldap://$SERVER ldap_search_base = dc=example,dc=com ldap_tls_cacert = /etc/openldap/certs/cacert.asc ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-263635006
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ One netgroupt test failed: Decrease the cache time out and add new entry for nisNetgroupTriple ``` echo "entry_cache_timeout = 60" >> /etc/sssd/sssd.conf // clear cache and restart sssd
cat << EOF | ldapmodify -x -h $SERVERS -p 389 -D "cn=Directory Manager" -w Secret123 -a dn: cn=QAUsers,ou=Netgroup,dc=example,dc=com objectClass: nisNetgroup objectClass: top cn: QAUsers nisNetgroupTriple: (testhost1,ng9000,$HOSTNAME) description: ALL QA users in my org EOF
// ensure that that netgroup with ng9000 exist getent -s sss netgroup QAUsers | grep ng9000"
// ensure that that netgroup with ng9001 does not exist getent -s sss netgroup QAUsers | grep ng9001" && exit 1
//add netgroup with ng9001 cat << EOF | ldapmodify -x -h $SERVERS -p 389 -D "cn=Directory Manager" -w $PASSWORD dn: cn=QAUsers,ou=Netgroup,dc=example,dc=com changetype: modify add: nisNetgroupTriple nisNetgroupTriple: (testhost2,ng9001,$HOSTNAME) EOF
python -c "from ctypes import CDLL; from ctypes.util import find_library; libc = CDLL(find_library('c')); x = libc.innetgr('QAUsers', None, 'ng9001', '$HOSTNAME'); assert x == 0"
sleep 70
python -c "from ctypes import CDLL; from ctypes.util import find_library; libc = CDLL(find_library('c')); x = libc.innetgr('QAUsers', None, 'ng9001', '$HOSTNAME'); assert x == 1;" ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-263635715
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Few multi domain tests failed: The first one was related to enumeration. Other failures probably as well. ``` [sssd domains = LOCAL,LDAP services = nss, pam
[nss] filter_groups = root filter_users = root
[pam] [domain/LOCAL] enumerate = TRUE id_provider = local max_id = 2010 min_id = 2000 use_fully_qualified_names = TRUE
[domain/LDAP] cache_credentials = FALSE enumerate = TRUE id_provider = ldap ldap_uri = ldaps://$SERVERS:636 ldap_tls_cacert = /etc/openldap/certs/cacert1.asc ldap_user_search_base = ou=people,dc=example,dc=com ldap_group_search_base = ou=groups,dc=example,dc=com max_id = 1010 min_id = 1000 use_fully_qualified_names = TRUE ``` and related test: ``` //add 2 ldap users user1 puser2 and two local users sss_useradd -u 2000 -h /home/user2000 -s /bin/bash user2000@LOCAL sss_useradd -u 2001 -h /home/user2001 -s /bin/bash user2001@LOCAL
//wailt a bit for finished enumeration sleep 10
getent -s sss passwd
# verify user enumeration # Users that should be returned RET=`getent -s sss passwd 2>&1` for item in puser1@LDAP puser2@LDAP user2000@LOCAL user2001@LOCAL ; do echo $RET | grep $item if [ $? -ne 0 ] ; then echo "ERROR: Expected $item user to be returned." else echo "Pass: $item user returned as expected." fi done ```
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-263638698
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ and I will check rest of ldap + AD related tests tomorrow. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-263641880
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ Hi, thank you for testing. All mentioned above is hopefully fixed, except the "Identical request is in progress". I am unable to reproduce this and I'm surprised that there is a issue since I did not touch this part of code at all. In my tests, all pending requests are handled after reconnection: ```
1034 (Wed Nov 30 12:57:52 2016) [sssd[nss]] [nss_dp_reconnect_init] (0x0020): Reconnected to the Data Provider. 1035 (Wed Nov 30 12:57:52 2016) [sssd[nss]] [rdp_message_send_internal] (0x0400): DP Request: /org/freedesktop/sssd/dataprovider org.freedesktop.sssd.DataProvider.Client.Register 1036 (Wed Nov 30 12:57:52 2016) [sssd[nss]] [sbus_add_timeout] (0x2000): 0x1701e70 1037 (Wed Nov 30 12:57:52 2016) [sssd[nss]] [handle_requests_after_reconnect] (0x1000): Will handle 1 requests after reconnect 1038 (Wed Nov 30 12:57:52 2016) [sssd[nss]] [cache_req_search_process_dp] (0x0040): CR #4: Data Provider Error: 3, 5, (null) ```
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-263857906
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ SSS_SEED user offline authentication failed: * block access to the ldap * sleep a bit (e.g. 4 seconds) * seed the SSSD cache with a user: sss_seed -D LDAP -n seeduser -u 10121 -g 10121 -c "Temporary" -h /home/temp-user -s /bin/bash * type some password twice for finishing sss_seed (e.g. SimplePassword123) * authenticate as a seeduser with password SimplePassword123
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-264885917
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ There is a simple domain config with case_preserving ``` [domain/LDAP] id_provider = ldap ldap_uri = ldap://$SERVER ldap_search_base = $DS_BASE_DN ldap_tls_cacert = /etc/openldap/certs/cacert.asc case_sensitive = preserving ``` Then you will add a service entry to LDAP server ``` dn: cn=Svc1,ou=Services,$DS_BASE_DN objectClass: ipService cn: Svc1_Alias ipServicePort: 1234 ipServiceProtocol: Tcp ``` and then try to run few getent commands for service ``` getent services SVC1 | grep Svc1 # passed getent services svc1_alias | grep Svc1 # passed getent services 1234/TCP # failed ``` I can see more failures related to getservbyport """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-264893034
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ There is also a crash for group enumeration ``` (Wed Nov 30 09:22:36 2016) [sssd[nss]] [cache_req_search_done] (0x0400): CR #0: Returning updated object [Groups enumeration] (Wed Nov 30 09:22:36 2016) [sssd[nss]] [cache_req_create_and_add_result] (0x0400): CR #0: Found 1 entries in domain LDAP (Wed Nov 30 09:22:36 2016) [sssd[nss]] [cache_req_done] (0x0400): CR #0: Finished: Success (Wed Nov 30 09:22:36 2016) [sssd[nss]] [nss_protocol_done] (0x4000): Sending reply: success (Wed Nov 30 09:22:36 2016) [sssd[nss]] [sss_dp_req_destructor] (0x0400): Deleting request: [0x7fc44877b5c0:2:*@LDAP] (Wed Nov 30 09:22:36 2016) [sssd[nss]] [nss_setent_timeout] (0x0400): Enumeration result object has expired. (Wed Nov 30 09:22:36 2016) [sssd[nss]] [talloc_log_fn] (0x0010): Bad talloc magic value - unknown value ```
I am so sorry I do not have more data. it's not obvious from test output what is wrong There is just an error: 'Group enumeration not retrieved within 5 attempts' which is obvious if nss responders crashes """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-264900328
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Regression test for https://fedorahosted.org/sssd/ticket/2865 failed or it was https://fedorahosted.org/sssd/ticket/2170
Following C-code was executed for existing and non-existing group ``` #include <netdb.h> #include <stdio.h>
int main(int argc, char** argv) { const long MAX = 10000000; const char* netgroup = "nonexisting"; int percentage = 0; int ret;
if (argc != 2) { fprintf(stderr, "Wrong count of arguments, using netgroup:[%s]\n", netgroup); } else { netgroup = argv[1]; }
long i; for(i=0; i<MAX; i++){ ret = setnetgrent(netgroup); if (! ret) { //perror("setnetgrent failed\n"); }
endnetgrent(); if (percentage != i * 100 / MAX) { fprintf(stderr, "%d%% ", ++percentage); } } return 0; }
``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-264905646
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ On 12/05/2016 04:36 PM, lslebodn wrote:
SSS_SEED user offline authentication failed:
- block access to the ldap
- sleep a bit (e.g. 4 seconds)
- seed the SSSD cache with a user: sss_seed -D LDAP -n seeduser -u 10121 -g 10121 -c "Temporary" -h /home/temp-user -s /bin/bash
- type some password twice for finishing sss_seed (e.g. SimplePassword123)
- authenticate as a seeduser with password SimplePassword123
This fails for me even on master: [pam_reply] (0x0200): pam_reply called with result [9]: Authentication service cannot retrieve authentication info.
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265117220
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ There is also a crash for group enumeration ``` (Wed Nov 30 09:22:36 2016) [sssd[nss]] [cache_req_search_done] (0x0400): CR #0: Returning updated object [Groups enumeration] (Wed Nov 30 09:22:36 2016) [sssd[nss]] [cache_req_create_and_add_result] (0x0400): CR #0: Found 1 entries in domain LDAP (Wed Nov 30 09:22:36 2016) [sssd[nss]] [cache_req_done] (0x0400): CR #0: Finished: Success (Wed Nov 30 09:22:36 2016) [sssd[nss]] [nss_protocol_done] (0x4000): Sending reply: success (Wed Nov 30 09:22:36 2016) [sssd[nss]] [sss_dp_req_destructor] (0x0400): Deleting request: [0x7fc44877b5c0:2:*@LDAP] (Wed Nov 30 09:22:36 2016) [sssd[nss]] [nss_setent_timeout] (0x0400): Enumeration result object has expired. (Wed Nov 30 09:22:36 2016) [sssd[nss]] [talloc_log_fn] (0x0010): Bad talloc magic value - unknown value ```
UPDATE: The crash is cause by `enum_cache_timeout = 0` in nss section ``` [nss] debug_level = 0xFFFF memcache_timeout = 0 # FIXME Remove once Bug 996610 is fixed enum_cache_timeout = 0 ``` and valgrind error: ``` ==15177== Process terminating with default action of signal 6 (SIGABRT) ==15177== at 0x32BEE32495: raise (raise.c:64) ==15177== by 0x32BEE33C74: abort (abort.c:92) ==15177== by 0x32C0E0248B: talloc_abort (talloc.c:399) ==15177== by 0x32C0E0A5E7: talloc_check_name (talloc.c:417) ==15177== by 0x408177: nss_setent_timeout (nss_enum.c:199) ==15177== by 0x32C4E08CC0: tevent_common_loop_timer_delay (tevent_timed.c:341) ==15177== by 0x32C4E09D01: epoll_event_loop_once (tevent_epoll.c:911) ==15177== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==15177== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==15177== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637) ==15177== by 0x32C4E082A5: std_event_loop_wait (tevent_standard.c:140) ==15177== by 0x32C564AEB2: server_loop (server.c:705) ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-264900328
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ When I was testing crash caused by enumeration I also found different valgrind errors. I double checked that these errors are not in master. ``` ==18958== 1 errors in context 1 of 1: ==18958== Syscall param socketcall.sendto(msg) points to uninitialised byte(s) ==18958== at 0x32BF20ED62: send (send.c:28) ==18958== by 0x41607E: sss_packet_send (responder_packet.c:238) ==18958== by 0x412B7A: client_fd_handler (responder_common.c:258) ==18958== by 0x32C4E09F05: epoll_event_loop_once (tevent_epoll.c:728) ==18958== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==18958== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==18958== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637) ==18958== by 0x32C4E082A5: std_event_loop_wait (tevent_standard.c:140) ==18958== by 0x32C564AEB2: server_loop (server.c:705) ==18958== by 0x4066F7: main (nsssrv.c:559) ==18958== Address 0x4da3148 is 120 bytes inside a block of size 608 alloc'd ==18958== at 0x4A06A2E: malloc (vg_replace_malloc.c:270) ==18958== by 0x32C0E03422: talloc_named_const (talloc.c:668) ==18958== by 0x41627A: sss_packet_new (responder_packet.c:84) ==18958== by 0x40982A: nss_protocol_reply (nss_protocol.c:84) ==18958== by 0x406B13: nss_getby_done (nss_cmd.c:338) ==18958== by 0x4198F9: cache_req_done (cache_req.c:690) ==18958== by 0x41A595: cache_req_search_done (cache_req_search.c:409) ==18958== by 0x415B6D: sss_dp_internal_get_done (responder_dp.c:813) ==18958== by 0x32C320E619: complete_pending_call_and_unlock (dbus-connection.c:2234) ==18958== by 0x32C321086E: dbus_connection_dispatch (dbus-connection.c:4397) ==18958== by 0x32C5641D7C: sbus_dispatch (sssd_dbus_connection.c:96) ==18958== by 0x32C4E08CC0: tevent_common_loop_timer_delay (tevent_timed.c:341) ``` and similar with realloc `==7607== Syscall param socketcall.sendto(msg) points to uninitialised byte(s) ==7607== at 0x32BF20ED62: send (send.c:28) ==7607== by 0x41607E: sss_packet_send (responder_packet.c:238) ==7607== by 0x412B7A: client_fd_handler (responder_common.c:258) ==7607== by 0x32C4E09F05: epoll_event_loop_once (tevent_epoll.c:728) ==7607== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==7607== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==7607== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637) ==7607== by 0x32C4E082A5: std_event_loop_wait (tevent_standard.c:140) ==7607== by 0x32C564AEB2: server_loop (server.c:705) ==7607== by 0x4066F7: main (nsssrv.c:559) ==7607== Address 0xe4c228c is 1,068 bytes inside a block of size 1,632 alloc'd ==7607== at 0x4A06C20: realloc (vg_replace_malloc.c:662) ==7607== by 0x32C0E099AA: _talloc_realloc (talloc.c:1880) ==7607== by 0x4163B9: sss_packet_grow (responder_packet.c:127) ==7607== by 0x40AF86: nss_protocol_fill_initgr (nss_protocol_grent.c:346) ==7607== by 0x409873: nss_protocol_reply (nss_protocol.c:91) ==7607== by 0x406B13: nss_getby_done (nss_cmd.c:338) ==7607== by 0x4198F9: cache_req_done (cache_req.c:690) ==7607== by 0x32C4E04867: tevent_common_loop_immediate (tevent_immediate.c:135) ==7607== by 0x32C4E09CF5: epoll_event_loop_once (tevent_epoll.c:906) ==7607== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==7607== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==7607== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637)`` ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265215397
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ When I was testing crash caused by enumeration I also found different valgrind errors. I double checked that these errors are not in master. ``` ==18958== 1 errors in context 1 of 1: ==18958== Syscall param socketcall.sendto(msg) points to uninitialised byte(s) ==18958== at 0x32BF20ED62: send (send.c:28) ==18958== by 0x41607E: sss_packet_send (responder_packet.c:238) ==18958== by 0x412B7A: client_fd_handler (responder_common.c:258) ==18958== by 0x32C4E09F05: epoll_event_loop_once (tevent_epoll.c:728) ==18958== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==18958== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==18958== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637) ==18958== by 0x32C4E082A5: std_event_loop_wait (tevent_standard.c:140) ==18958== by 0x32C564AEB2: server_loop (server.c:705) ==18958== by 0x4066F7: main (nsssrv.c:559) ==18958== Address 0x4da3148 is 120 bytes inside a block of size 608 alloc'd ==18958== at 0x4A06A2E: malloc (vg_replace_malloc.c:270) ==18958== by 0x32C0E03422: talloc_named_const (talloc.c:668) ==18958== by 0x41627A: sss_packet_new (responder_packet.c:84) ==18958== by 0x40982A: nss_protocol_reply (nss_protocol.c:84) ==18958== by 0x406B13: nss_getby_done (nss_cmd.c:338) ==18958== by 0x4198F9: cache_req_done (cache_req.c:690) ==18958== by 0x41A595: cache_req_search_done (cache_req_search.c:409) ==18958== by 0x415B6D: sss_dp_internal_get_done (responder_dp.c:813) ==18958== by 0x32C320E619: complete_pending_call_and_unlock (dbus-connection.c:2234) ==18958== by 0x32C321086E: dbus_connection_dispatch (dbus-connection.c:4397) ==18958== by 0x32C5641D7C: sbus_dispatch (sssd_dbus_connection.c:96) ==18958== by 0x32C4E08CC0: tevent_common_loop_timer_delay (tevent_timed.c:341) ``` and similar with realloc ``` ==7607== Syscall param socketcall.sendto(msg) points to uninitialised byte(s) ==7607== at 0x32BF20ED62: send (send.c:28) ==7607== by 0x41607E: sss_packet_send (responder_packet.c:238) ==7607== by 0x412B7A: client_fd_handler (responder_common.c:258) ==7607== by 0x32C4E09F05: epoll_event_loop_once (tevent_epoll.c:728) ==7607== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==7607== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==7607== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637) ==7607== by 0x32C4E082A5: std_event_loop_wait (tevent_standard.c:140) ==7607== by 0x32C564AEB2: server_loop (server.c:705) ==7607== by 0x4066F7: main (nsssrv.c:559) ==7607== Address 0xe4c228c is 1,068 bytes inside a block of size 1,632 alloc'd ==7607== at 0x4A06C20: realloc (vg_replace_malloc.c:662) ==7607== by 0x32C0E099AA: _talloc_realloc (talloc.c:1880) ==7607== by 0x4163B9: sss_packet_grow (responder_packet.c:127) ==7607== by 0x40AF86: nss_protocol_fill_initgr (nss_protocol_grent.c:346) ==7607== by 0x409873: nss_protocol_reply (nss_protocol.c:91) ==7607== by 0x406B13: nss_getby_done (nss_cmd.c:338) ==7607== by 0x4198F9: cache_req_done (cache_req.c:690) ==7607== by 0x32C4E04867: tevent_common_loop_immediate (tevent_immediate.c:135) ==7607== by 0x32C4E09CF5: epoll_event_loop_once (tevent_epoll.c:906) ==7607== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==7607== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==7607== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637)`` ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265215397
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ On (06/12/16 02:40), Pavel Březina wrote:
On 12/05/2016 04:36 PM, lslebodn wrote:
SSS_SEED user offline authentication failed:
- block access to the ldap
- sleep a bit (e.g. 4 seconds)
- seed the SSSD cache with a user: sss_seed -D LDAP -n seeduser -u 10121 -g 10121 -c "Temporary" -h /home/temp-user -s /bin/bash
- type some password twice for finishing sss_seed (e.g. SimplePassword123)
- authenticate as a seeduser with password SimplePassword123
This fails for me even on master: [pam_reply] (0x0200): pam_reply called with result [9]: Authentication service cannot retrieve authentication info.
It just mean that I didn't provide reliable reproducer because it works on master.
FYI: it is caused by valgrind error "Syscall param socketcall.sendto(msg) points to uninitialised byte(s)"
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265244156
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ Enumeration, services and memory leak were fixed.
* Enumeration issue was caused by calling `talloc_get_type()` on non-talloc pointer. * Services issue was caused by not lowercasing protocol name on case-insensitive domains. * Memory leak was caused by not freeing subrequest after calling `_recv()` * The valgrind issues were caused by initgroups when we were leaving an uninitialized value when no original primary gid was inserted. Fixed with: ```c if (orig_gid == 0) { /* Initialize allocated memory to be safe and make valgrind happy. */ SAFEALIGN_SET_UINT32(&body[rp], 0, &rp); } else { /* Insert original primary group into the result. */ SAFEALIGN_COPY_UINT32(&body[rp], &orig_gid, &rp); num_results++; } ```
I can successfully log-in with seeded user now without any valgrind error, but I was able to login with this user even before these changes, so I hope this is fixed as well. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265446392
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Looks like there is use after free in latest version. sorry do not have a reproducer yet; just a valgrind output ``` ==6612== 18 errors in context 1 of 1: ==6612== Invalid read of size 8 ==6612== at 0x408748: nss_setent_internal_done (nss_enum.c:173) ==6612== by 0x419A19: cache_req_done (cache_req.c:690) ==6612== by 0x41A6B5: cache_req_search_done (cache_req_search.c:409) ==6612== by 0x415C8D: sss_dp_internal_get_done (responder_dp.c:813) ==6612== by 0x32C320E619: complete_pending_call_and_unlock (dbus-connection.c:2234) ==6612== by 0x32C321086E: dbus_connection_dispatch (dbus-connection.c:4397) ==6612== by 0x5068D7C: sbus_dispatch (sssd_dbus_connection.c:96) ==6612== by 0x32C4E08CC0: tevent_common_loop_timer_delay (tevent_timed.c:341) ==6612== by 0x32C4E09D01: epoll_event_loop_once (tevent_epoll.c:911) ==6612== by 0x32C4E08335: std_event_loop_once (tevent_standard.c:114) ==6612== by 0x32C4E03C3C: _tevent_loop_once (tevent.c:533) ==6612== by 0x32C4E03CBA: tevent_common_loop_wait (tevent.c:637) ==6612== Address 0xedcb820 is 544 bytes inside a block of size 805 free'd ==6612== at 0x4A06430: free (vg_replace_malloc.c:446) ==6612== by 0x32C0E07886: _talloc_free_internal (talloc.c:1116) ==6612== by 0x4077A7: nss_setnetgrent_done (nss_cmd.c:566) ==6612== by 0x408747: nss_setent_internal_done (nss_enum.c:172) ==6612== by 0x419A19: cache_req_done (cache_req.c:690) ==6612== by 0x41A6B5: cache_req_search_done (cache_req_search.c:409) ==6612== by 0x415C8D: sss_dp_internal_get_done (responder_dp.c:813) ==6612== by 0x32C320E619: complete_pending_call_and_unlock (dbus-connection.c:2234) ==6612== by 0x32C321086E: dbus_connection_dispatch (dbus-connection.c:4397) ==6612== by 0x5068D7C: sbus_dispatch (sssd_dbus_connection.c:96) ==6612== by 0x32C4E08CC0: tevent_common_loop_timer_delay (tevent_timed.c:341) ==6612== by 0x32C4E09D01: epoll_event_loop_once (tevent_epoll.c:911) ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265686249
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ On 12/08/2016 09:50 AM, lslebodn wrote:
Looks like there is use after free in latest version. sorry do not have a reproducer yet; just a valgrind output
nss_setnetgrent_done (nss_cmd.c:566) ==6612== by 0x408747:
It seems to be a netgroup search. Do you have sssd.conf?
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265687913
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Another crash from AD related test. 0xbe was initialized either by `MALLOC_PERTRUB` or `TALLOC_FREE_FILL` ``` #0 0x00007fa2dd6a8423 in setent_notify (list=0xbebebebebebebede, err=err@entry=0) at src/responder/common/responder_cmd.c:222 #1 0x00007fa2dd6a84d7 in setent_notify_done (list=<optimized out>) at src/responder/common/responder_cmd.c:251 #2 0x00007fa2dd69dfd8 in nss_setent_internal_done (subreq=0x0) at src/responder/nss/nss_enum.c:173 #3 0x00007fa2dd6b0195 in cache_req_done (subreq=0x0) at src/responder/common/cache_req/cache_req.c:690 #4 0x00007fa2dd6b0a4e in cache_req_search_done (subreq=<optimized out>) at src/responder/common/cache_req/cache_req_search.c:409 #5 0x00007fa2dd6ab4ac in sss_dp_internal_get_done (pending=<optimized out>, ptr=<optimized out>) at src/responder/common/responder_dp.c:813 #6 0x00007fa2dc56c862 in complete_pending_call_and_unlock () from /lib64/libdbus-1.so.3 #7 0x00007fa2dc56fb51 in dbus_connection_dispatch () from /lib64/libdbus-1.so.3 #8 0x00007fa2dce29cf2 in sbus_dispatch (ev=0x7fa2ded16a30, te=<optimized out>, tv=..., data=<optimized out>) at src/sbus/sssd_dbus_connection.c:96 #9 0x00007fa2d94c4b4f in tevent_common_loop_timer_delay (ev=ev@entry=0x7fa2ded16a30) at ../tevent_timed.c:341 #10 0x00007fa2d94c5b5a in epoll_event_loop_once (ev=0x7fa2ded16a30, location=<optimized out>) at ../tevent_epoll.c:911 #11 0x00007fa2d94c4257 in std_event_loop_once (ev=0x7fa2ded16a30, location=0x7fa2dce52787 "src/util/server.c:705") at ../tevent_standard.c:114 #12 0x00007fa2d94c040d in _tevent_loop_once (ev=ev@entry=0x7fa2ded16a30, location=location@entry=0x7fa2dce52787 "src/util/server.c:705") at ../tevent.c:533 #13 0x00007fa2d94c05ab in tevent_common_loop_wait (ev=0x7fa2ded16a30, location=0x7fa2dce52787 "src/util/server.c:705") at ../tevent.c:637 #14 0x00007fa2d94c41f7 in std_event_loop_wait (ev=0x7fa2ded16a30, location=0x7fa2dce52787 "src/util/server.c:705") at ../tevent_standard.c:140 #15 0x00007fa2dce35073 in server_loop (main_ctx=0x7fa2ded17e80) at src/util/server.c:705 #16 0x00007fa2dd69b357 in main (argc=6, argv=<optimized out>) at src/responder/nss/nsssrv.c:559 ```
There are more failures in ad forest test; will provide reproducers """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265708774
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ The crash was caused by the same issue as the valgrind log. It is fixed with:
```c diff --git a/src/responder/nss/nss_enum.c b/src/responder/nss/nss_enum.c index 5f29d6d..9c922aa 100644 --- a/src/responder/nss/nss_enum.c +++ b/src/responder/nss/nss_enum.c @@ -118,6 +118,7 @@ static void nss_setent_internal_done(struct tevent_req *subreq) { struct cache_req_result **result; struct nss_setent_internal_state *state; + struct setent_req_list **notify_list; struct tevent_req *req; errno_t ret; errno_t tret; @@ -168,12 +169,20 @@ static void nss_setent_internal_done(struct tevent_req *subreq) ret = EOK;
done: + /* We want to finish the requests in correct order, this was the + * first request, notify_list contain the subsequent request. + * + * Because callback invoked from tevent_req_done will free state, + * we must remember notify_list explicitly to avoid segfault. + */ + notify_list = &state->enum_ctx->notify_list; + if (ret == EOK) { tevent_req_done(req); - setent_notify_done(&state->enum_ctx->notify_list); + setent_notify_done(notify_list); } else { - setent_notify(&state->enum_ctx->notify_list, ret); tevent_req_error(req, ret); + setent_notify(notify_list, ret); } } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265718180
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ The crash was caused by the same issue as the valgrind log. It is fixed with:
```c diff --git a/src/responder/nss/nss_enum.c b/src/responder/nss/nss_enum.c index 5f29d6d..9c922aa 100644 --- a/src/responder/nss/nss_enum.c +++ b/src/responder/nss/nss_enum.c @@ -118,6 +118,7 @@ static void nss_setent_internal_done(struct tevent_req *subreq) { struct cache_req_result **result; struct nss_setent_internal_state *state; + struct setent_req_list **notify_list; struct tevent_req *req; errno_t ret; errno_t tret; @@ -168,12 +169,20 @@ static void nss_setent_internal_done(struct tevent_req *subreq) ret = EOK;
done: + /* We want to finish the requests in correct order, this was the + * first request, notify_list contain the subsequent request. + * + * Because callback invoked from tevent_req_done will free state, + * we must remember notify_list explicitly to avoid segfault. + */ + notify_list = &state->enum_ctx->notify_list; + if (ret == EOK) { tevent_req_done(req); - setent_notify_done(&state->enum_ctx->notify_list); + setent_notify_done(notify_list); } else { - setent_notify(&state->enum_ctx->notify_list, ret); tevent_req_error(req, ret); + setent_notify(notify_list, ret); } } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-265718180
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ Hi, I added: https://github.com/SSSD/sssd/pull/89/commits/a7dd2b118170e6dacaf41dba4874ad5...
This should fix the AD failures, please test it again. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266399727
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ I saw a crash as part of ad_testing. But it seems to be unrelated to AD tests. ``` #0 __strlen_sse2_pminub () at ../sysdeps/x86_64/multiarch/strlen-sse2-pminub.S:38 #1 0x00007f69bd6c5384 in sss_tc_utf8_str_tolower (mem_ctx=mem_ctx@entry=0x7f69bfce3b80, s=0x0) at src/util/sss_tc_utf8.c:31 #2 0x00007f69bd6c2df9 in sss_get_cased_name (mem_ctx=mem_ctx@entry=0x7f69bfce3b80, orig_name=<optimized out>, case_sensitive=<optimized out>) at src/util/usertools.c:516 #3 0x00007f69bdf41d7b in cache_req_svc_by_name_prepare_domain_data (cr=0x7f69bfcee0c0, data=0x7f69bfce3c70, domain=0x7f69bfcd69c0) at src/responder/common/cache_req/plugins/cache_req_svc_by_name.c:57 #4 0x00007f69bdf3d188 in cache_req_prepare_domain_data (domain=0x7f69bfcd69c0, cr=0x7f69bfcee0c0) at src/responder/common/cache_req/cache_req.c:200 #5 cache_req_set_domain (domain=0x7f69bfcd69c0, cr=0x7f69bfcee0c0) at src/responder/common/cache_req/cache_req.c:241 #6 cache_req_next_domain (req=0x7f69bfcede90) at src/responder/common/cache_req/cache_req.c:536 #7 0x00007f69bdf3d90f in cache_req_input_parsed (subreq=0x7f69bfcee1d0) at src/responder/common/cache_req/cache_req.c:462 #8 0x00007f69bdf3af9e in sss_dp_get_domains_process (subreq=0x0) at src/responder/common/responder_get_domains.c:255 #9 0x00007f69bdf394bc in sss_dp_internal_get_done (pending=<optimized out>, ptr=<optimized out>) at src/responder/common/responder_dp.c:813 #10 0x00007f69bcdf9862 in complete_pending_call_and_unlock (connection=connection@entry=0x7f69bfcdbb60, pending=0x7f69bfce2170, message=message@entry=0x7f69bfcdd620) at dbus-connection.c:2314 #11 0x00007f69bcdfcb51 in dbus_connection_dispatch (connection=connection@entry=0x7f69bfcdbb60) at dbus-connection.c:4580 #12 0x00007f69bd6b70b2 in sbus_dispatch (ev=0x7f69bfccda30, te=<optimized out>, tv=..., data=<optimized out>) at src/sbus/sssd_dbus_connection.c:96 #13 0x00007f69b9d51b4f in tevent_common_loop_timer_delay (ev=ev@entry=0x7f69bfccda30) at ../tevent_timed.c:341 #14 0x00007f69b9d52b5a in epoll_event_loop_once (ev=0x7f69bfccda30, location=<optimized out>) at ../tevent_epoll.c:911 #15 0x00007f69b9d51257 in std_event_loop_once (ev=0x7f69bfccda30, location=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent_standard.c:114 #16 0x00007f69b9d4d40d in _tevent_loop_once (ev=ev@entry=0x7f69bfccda30, location=location@entry=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent.c:533 #17 0x00007f69b9d4d5ab in tevent_common_loop_wait (ev=0x7f69bfccda30, location=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent.c:637 #18 0x00007f69b9d511f7 in std_event_loop_wait (ev=0x7f69bfccda30, location=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent_standard.c:140 #19 0x00007f69bd6c2433 in server_loop (main_ctx=0x7f69bfccee80) at src/util/server.c:705 #20 0x00007f69bdf29357 in main (argc=6, argv=<optimized out>) at src/responder/nss/nsssrv.c:559 ```
It is a dereference of NULL pointer ``` (gdb) frame 3 #3 0x00007f69bdf41d7b in cache_req_svc_by_name_prepare_domain_data (cr=0x7f69bfcee0c0, data=0x7f69bfce3c70, domain=0x7f69bfcd69c0) at src/responder/common/cache_req/plugins/cache_req_svc_by_name.c:57 57 protocol = sss_get_cased_name(tmp_ctx, cr->data->svc.protocol.name, (gdb) p name $3 = 0x7f69bfcefb80 "rpcbind" (gdb) p cr->data->svc.protocol $4 = {name = 0x0, lookup = 0x0} ```
I assume this is unrelated to AD_tests but to the other unrelated thing which was done as part of test """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266443860
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ I saw a crash as part of ad_testing. But it seems to be unrelated to AD tests. ``` #0 __strlen_sse2_pminub () at ../sysdeps/x86_64/multiarch/strlen-sse2-pminub.S:38 #1 0x00007f69bd6c5384 in sss_tc_utf8_str_tolower (mem_ctx=mem_ctx@entry=0x7f69bfce3b80, s=0x0) at src/util/sss_tc_utf8.c:31 #2 0x00007f69bd6c2df9 in sss_get_cased_name (mem_ctx=mem_ctx@entry=0x7f69bfce3b80, orig_name=<optimized out>, case_sensitive=<optimized out>) at src/util/usertools.c:516 #3 0x00007f69bdf41d7b in cache_req_svc_by_name_prepare_domain_data (cr=0x7f69bfcee0c0, data=0x7f69bfce3c70, domain=0x7f69bfcd69c0) at src/responder/common/cache_req/plugins/cache_req_svc_by_name.c:57 #4 0x00007f69bdf3d188 in cache_req_prepare_domain_data (domain=0x7f69bfcd69c0, cr=0x7f69bfcee0c0) at src/responder/common/cache_req/cache_req.c:200 #5 cache_req_set_domain (domain=0x7f69bfcd69c0, cr=0x7f69bfcee0c0) at src/responder/common/cache_req/cache_req.c:241 #6 cache_req_next_domain (req=0x7f69bfcede90) at src/responder/common/cache_req/cache_req.c:536 #7 0x00007f69bdf3d90f in cache_req_input_parsed (subreq=0x7f69bfcee1d0) at src/responder/common/cache_req/cache_req.c:462 #8 0x00007f69bdf3af9e in sss_dp_get_domains_process (subreq=0x0) at src/responder/common/responder_get_domains.c:255 #9 0x00007f69bdf394bc in sss_dp_internal_get_done (pending=<optimized out>, ptr=<optimized out>) at src/responder/common/responder_dp.c:813 #10 0x00007f69bcdf9862 in complete_pending_call_and_unlock (connection=connection@entry=0x7f69bfcdbb60, pending=0x7f69bfce2170, message=message@entry=0x7f69bfcdd620) at dbus-connection.c:2314 #11 0x00007f69bcdfcb51 in dbus_connection_dispatch (connection=connection@entry=0x7f69bfcdbb60) at dbus-connection.c:4580 #12 0x00007f69bd6b70b2 in sbus_dispatch (ev=0x7f69bfccda30, te=<optimized out>, tv=..., data=<optimized out>) at src/sbus/sssd_dbus_connection.c:96 #13 0x00007f69b9d51b4f in tevent_common_loop_timer_delay (ev=ev@entry=0x7f69bfccda30) at ../tevent_timed.c:341 #14 0x00007f69b9d52b5a in epoll_event_loop_once (ev=0x7f69bfccda30, location=<optimized out>) at ../tevent_epoll.c:911 #15 0x00007f69b9d51257 in std_event_loop_once (ev=0x7f69bfccda30, location=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent_standard.c:114 #16 0x00007f69b9d4d40d in _tevent_loop_once (ev=ev@entry=0x7f69bfccda30, location=location@entry=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent.c:533 #17 0x00007f69b9d4d5ab in tevent_common_loop_wait (ev=0x7f69bfccda30, location=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent.c:637 #18 0x00007f69b9d511f7 in std_event_loop_wait (ev=0x7f69bfccda30, location=0x7f69bd6e0167 "src/util/server.c:705") at ../tevent_standard.c:140 #19 0x00007f69bd6c2433 in server_loop (main_ctx=0x7f69bfccee80) at src/util/server.c:705 #20 0x00007f69bdf29357 in main (argc=6, argv=<optimized out>) at src/responder/nss/nsssrv.c:559 ```
It is a dereference of NULL pointer ``` (gdb) frame 3 #3 0x00007f69bdf41d7b in cache_req_svc_by_name_prepare_domain_data (cr=0x7f69bfcee0c0, data=0x7f69bfce3c70, domain=0x7f69bfcd69c0) at src/responder/common/cache_req/plugins/cache_req_svc_by_name.c:57 57 protocol = sss_get_cased_name(tmp_ctx, cr->data->svc.protocol.name, (gdb) p name $3 = 0x7f69bfcefb80 "rpcbind" (gdb) p cr->data->svc.protocol $4 = {name = 0x0, lookup = 0x0} ```
I assume this is unrelated to AD_tests but to the other unrelated thing which was done as part of test
UPDATE: I can reproduce the crash after restarting sssd and calling `getent services -s sss rpcbind` The sssd.conf is unrelated but here you are :-) ``` [nss] filter_groups = root filter_users = root default_shell = /bin/bash override_homedir = /home/%u
[domain/sssdad.com] ldap_purge_cache_timeout = 0 debug_level = 0xFFF0 id_provider = ad ad_domain = sssdad.com ``` """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266443860
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ Fixed. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266708967
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ Just for the record; the last update fixed netgroup memory leak for long living clients """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266873650
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ I had a few inline comments; mostly nitpicks CI passed (rawhide failure is unrelated http://sssd-ci.duckdns.org/logs/job/58/78/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266873811
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ I almost forgot to write the most important message. AD related tests passed. LDAP related tests passed; there are some intermittent failures; but maybe not related to these patches. I was not able to properly finish testing with IPA due to issues with IPA on CentOS 7.3
Summary: Functional ACK """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266897816
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ On 12/13/2016 10:01 PM, lslebodn wrote:
*@lslebodn* commented on this pull request.
In src/responder/nss/nss_protocol_grent.c https://github.com/SSSD/sssd/pull/89#pullrequestreview-12777658:
- int i, j;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory!\n");
return ENOMEM;
- }
- members[0] = nss_get_group_members(domain, msg);
- members[1] = nss_get_group_ghosts(domain, msg, group_name);
- sss_packet_get_body(packet, &body, &body_len);
- num_members = 0;
- for (i = 0; i < sizeof(members)/sizeof(members[0]); i++) {
el = members[i];
it might be good have spaces around division and from performance POV, you might store it to variable/constant We needn't divide it every time.
I expect this to be optimized during compilation since it is constant and can be computed during compile time.
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-266990482
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ All your comments were addressed. I also added https://github.com/SSSD/sssd/pull/89/commits/cf975c06bfb6710d8254b052c42161e... This patch should solve the sss_seed tool problem with negative cache. Now we store the negative cache record only of DP requests succeeded which mimics the original behaviour. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-267002033
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ ACK
Time for review for others expired :-). But you can still after commit. """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-268099531
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/89 Author: pbrezina Title: #89: nss: rewrite nss responder so it uses cache_req Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/89/head:pr89 git checkout pr89
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
lslebodn commented: """ I fixed few coding style issues( indentation 80 columns limit ...) and removing files from makefile(distcheck failed with HEAD~1). I hope you don't mind.
* 075d89886666d3b608355d8f235b411051a9d22e * 8d5292227a8d1ab9c6aa5b88d8ac8655cd1223e5 * 4049b63f8c67ada17b453463b0451ca6be3d5de4 * a5a3bbb0bbaeb8946c228c2fb7f0cf450595dd3e * 7162dc780fe9458018c577f6f1638522d74f63b0 * 87d85db07bdd39081f558e2f7e891cc0065e3a0a * 0713b92ec9f10b6dd913dc56cbc7845d1b025ccb * 2d12aae08aff6c17c1edb107a204c54d9acfe08d * 122228f496f1a914b0763fb47a50854f168dc3b4 * bad19f954d1946fb601df253059afbe011bcc914 * 817e3ec31bbdb5447f4ffcd3302c558283b90943 * 8f895983e8d24b3edde4f695621f6b9a2fd20923 * 3be2628d8aba6aeb99ac1484da990f1fad8169ec * 488518dde58724daa13b9216a0f1af6e0ba5401f * c85f751893e43d992b6fac7c83679db05558d1ca * 1b33f4d5c564141b888071a342403230a71983cb * 7a2ca8d776df685bddbb64370181fb32d776f676 * 7be55c7de03da57f43fae3db7e6114eebb117a2e * 4e2c15e6b7c4015fa787f8c624c2ec10153e99f6 * 6b159f14f69134bba8510a6b50ab62493a23a73f * 0ae7e46a3990c47873fca879a9395e3ce00d9150 * c2fc9459c31cb1192ab3c15ce4df1c150e99bf95 * 2e13817e64ff1e0e47dc844be501f2d3ab299f34 * 12d771585a84a7523a5b7d9cf502d4bcddecb9b9 * a79acee185654d110c0e35ba351368d664e4e53d * 9c98397b6431b6b02bdfdb0540bac6a3eb00b0e3 * 3df5c41c19ef852021819954a2db1d067844d136 * b206e1abb7f6ea373d12537b3338552aed6b656d * f63607bfcc01ad426efa20ed8ec65f429c9b2bd6 * 39b4feb503082cbbd036b2dcd741fe2ffe4aed76 * 7b293a5095ef3e63cd2e3f2ff01b7484bf6dcd38 * 35d1617655ca7270a23a716f60e7a4497cbba4ba * e4b147ed01c8476d36ce356ee53899870d84351d * 1d5e693461c0f6d645e850c6a0cd895c9db3927d * a22b0af1993a489c9c0e66fdc1083f43b410d12c """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-268100262
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
jhrozek commented: """ @pbrezina @lslebodn is there any more work needed on https://fedorahosted.org/sssd/ticket/1126 or https://fedorahosted.org/sssd/ticket/2320 ? Do we anticipate to work on other responders?
The end goal is to support https://fedorahosted.org/sssd/ticket/843 and https://fedorahosted.org/sssd/ticket/3001 """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-268193885
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
spbnick commented: """ Not a review, but rather a question to @pbrezina: why do some "plugin" functions receive both cache_req_data and cache_req, which also references cache_req_data, at the same time? Are those referring to the same cache_req_data? """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-268204367
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
spbnick commented: """ Another question is cache_req going to be used by PAM (if this question even makes sense)? """
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-268206740
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ @jhrozek * #1126 -- pam, ssh and pac (?) responders needs to be amended, but the change there is not that huge. * #2320 -- not sure if this is needed anywhere with cache req
@spbnick -- it is the same data pointer which is passed directly to the function as a shortcut, so we can use `data` and not `cr->data`. Cache_req is going to be used in PAM as well.
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-269951344
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
jhrozek commented: """ On Mon, Jan 02, 2017 at 01:53:23AM -0800, Pavel Březina wrote:
@jhrozek
- #1126 -- pam, ssh and pac (?) responders needs to be amended, but the change there is not that huge.
OK, then let's keep the ticket open. Do you plan on working on this one or would you prefer if someone else took a look?
- #2320 -- not sure if this is needed anywhere with cache req
I agree if other responders start using cache_req, we don't need this anymore.
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-270023513
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
pbrezina commented: """ On 01/02/2017 10:44 PM, Jakub Hrozek wrote:
On Mon, Jan 02, 2017 at 01:53:23AM -0800, Pavel Březina wrote:
@jhrozek
- #1126 -- pam, ssh and pac (?) responders needs to be amended, but
the change there is not that huge.
OK, then let's keep the ticket open. Do you plan on working on this one or would you prefer if someone else took a look?
We can split work. I'll do ssh responder since I already started. Someone else can do pam and pac.
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-270082467
URL: https://github.com/SSSD/sssd/pull/89 Title: #89: nss: rewrite nss responder so it uses cache_req
fidencio commented: """ On Tue, Jan 3, 2017 at 11:06 AM, Pavel Březina notifications@github.com wrote:
On 01/02/2017 10:44 PM, Jakub Hrozek wrote:
On Mon, Jan 02, 2017 at 01:53:23AM -0800, Pavel Březina wrote:
@jhrozek
- #1126 -- pam, ssh and pac (?) responders needs to be amended, but
the change there is not that huge.
OK, then let's keep the ticket open. Do you plan on working on this one or would you prefer if someone else took a look?
We can split work. I'll do ssh responder since I already started. Someone else can do pam and pac.
Okay, I'll take a look on these then.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/89#issuecomment-270082467, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4ettNNYFyooMiz8sKaoJ51M8phH70ks5rOh2dgaJpZM4K7nfP .
"""
See the full comment at https://github.com/SSSD/sssd/pull/89#issuecomment-270111255
sssd-devel@lists.fedorahosted.org