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:
Thanks. This helped. New patches attached, also including two more small
patches.