On (02/12/15 11:05), Pavel Březina wrote:
>On 12/01/2015 02:02 PM, Lukas Slebodnik wrote:
>>On (24/11/15 13:23), Pavel Březina wrote:
>>>Hi,
>>>I'm sending some sudo provider patches. I wanted to fix/improve things in the
>>>ldap sudo provider prior my work on ipa provider so I get familiar with it
>>>again and avoid making the same mistakes.
>>>
>>>It fixes tevent style, shuffles the code around a little bit, convert
>>>periodic task to use be_ptask module, renew hostinfo when needed, fix
>>>sdap_id_op logic, recude code duplication, remove dead code, simplify error
>>>handling, etc.
>>>
>>>Ticket fixed:
>>>https://fedorahosted.org/sssd/ticket/1943
>>>https://fedorahosted.org/sssd/ticket/2672
>>>
>>>I let Dan run downstream tests on those patches. We had to fix one test that
>>>was prone to a race condition which my patches revealed, but everything is
>>>green now.
>>>
>>
>>I got following valgrind errors with patches
>>
>>==17279== 1 errors in context 3 of 7:
>>==17279== Conditional jump or move depends on uninitialised value(s)
>>==17279== at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
>>==17279== by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
>>==17279== by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
>>==17279== by 0x89B4201: _tevent_req_error (tevent_req.c:167)
>>==17279== by 0x13DA3321: sdap_sudo_load_sudoers_done
>>(sdap_async_sudo.c:170)
>>==17279== by 0x89B4201: _tevent_req_error (tevent_req.c:167)
>>==17279== by 0x89B4201: _tevent_req_error (tevent_req.c:167)
>>==17279== by 0x13D71ADB: generic_ext_search_handler.isra.3
>>(sdap_async.c:1651)
>>==17279== by 0x89B3923: tevent_common_loop_immediate
>>(tevent_immediate.c:135)
>>==17279== by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
>>==17279== by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
>>==17279== by 0x89B30FC: _tevent_loop_once (tevent.c:533)
>>==17279== by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
>>==17279==
>>==17279==
>>==17279== 1 errors in context 4 of 7:
>>==17279== Conditional jump or move depends on uninitialised value(s)
>>==17279== at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
>>==17279== by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
>>==17279== by 0x89B4201: _tevent_req_error (tevent_req.c:167)
>>==17279== by 0x13DA3321: sdap_sudo_load_sudoers_done
>>(sdap_async_sudo.c:170)
>>==17279== by 0x89B4201: _tevent_req_error (tevent_req.c:167)
>>==17279== by 0x89B4201: _tevent_req_error (tevent_req.c:167)
>>==17279== by 0x13D71ADB: generic_ext_search_handler.isra.3
>>(sdap_async.c:1651)
>>==17279== by 0x89B3923: tevent_common_loop_immediate
>>(tevent_immediate.c:135)
>>==17279== by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
>>==17279== by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
>>==17279== by 0x89B30FC: _tevent_loop_once (tevent.c:533)
>>==17279== by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
>>==17279== by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)
>
>I can't see a codepath where usn could be uninitialized, do you?
I didn't try but static analysers helped me,
at least I hope it will help you :-)
Maybe, once you will learn how to use them :-) :-) :-)
Error: UNINIT (CWE-457): [#def1]
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690: var_decl: Declaring
variable "usn" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750: uninit_use_in_call:
Using uninitialized value "usn" when calling "sdap_sudo_set_usn".
# 748|
# 749| /* remember new usn */
# 750|-> sdap_sudo_set_usn(state->srv_opts, usn);
# 751|
# 752| ret = EOK;
Error: CLANG_WARNING: [#def2]
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: warning: Function call
argument is an uninitialized value
# sdap_sudo_set_usn(state->srv_opts, usn);
# ^ ~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690:5: note: 'usn' declared
without an initial value
# char *usn;
# ^~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:9: note: Assuming
'dp_error' is not equal to 0
# if (dp_error == DP_ERR_OK && ret != EOK) {
# ^~~~~~~~~~~~~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:31: note: Left side of
'&&' is false
# if (dp_error == DP_ERR_OK && ret != EOK) {
# ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:9: note: Assuming 'ret'
is equal to 0
# if (ret != EOK) {
# ^~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:5: note: Taking false
branch
# if (ret != EOK) {
# ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:9: note: Assuming 'ret'
is equal to 0
# if (ret != EOK) {
# ^~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:5: note: Taking false
branch
# if (ret != EOK) {
# ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Calling
'sdap_sudo_store_sudoers'
# ret = sdap_sudo_store_sudoers(state, state->domain,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:285:26: note: Left side of
'||' is true
# if (rules_count == 0 || rules == NULL) {
# ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Returning from
'sdap_sudo_store_sudoers'
# ret = sdap_sudo_store_sudoers(state, state->domain,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:735:5: note: Taking false
branch
# if (ret != EOK) {
# ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:9: note: Assuming 'ret'
is equal to 0
# if (ret != EOK) {
# ^~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:5: note: Taking false
branch
# if (ret != EOK) {
# ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: note: Function call
argument is an uninitialized value
# sdap_sudo_set_usn(state->srv_opts, usn);
# ^ ~~~
# 748|
# 749| /* remember new usn */
# 750|-> sdap_sudo_set_usn(state->srv_opts, usn);
# 751|
# 752| ret = EOK;
Error: UNINIT (CWE-457): [#def3]
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:130: var_decl: Declaring variable
"deleted" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:142: uninit_use: Using
uninitialized value "deleted".
# 140| case BE_REQ_SUDO_RULES:
# 141| ret = sdap_sudo_rules_refresh_recv(req, &dp_error, &deleted);
# 142|-> if (ret == EOK && deleted == true) {
# 143| ret = ENOENT;
# 144| }
Error: UNINIT (CWE-457): [#def4]
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:129: var_decl: Declaring variable
"dp_error" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:155: uninit_use_in_call: Using
uninitialized value "dp_error" when calling "sdap_handler_done".
# 153|
# 154| talloc_zfree(req);
# 155|-> sdap_handler_done(be_req, dp_error, ret, strerror(ret));
# 156| }
# 157|
Error: UNINIT (CWE-457): [#def5]
sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:388: var_decl: Declaring
variable "downloaded_rules_num" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:401: uninit_use: Using
uninitialized value "downloaded_rules_num".
# 399| }
# 400|
# 401|-> state->deleted = downloaded_rules_num != state->num_rules ? true
: false;
# 402|
# 403| done:
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
