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