On Mon, Dec 16, 2013 at 10:40:46AM +0100, Pavel Březina wrote:
On 12/13/2013 10:35 AM, Jakub Hrozek wrote:
>On Thu, Dec 12, 2013 at 09:54:51PM +0100, Jakub Hrozek wrote:
>>On Thu, Dec 12, 2013 at 01:20:59PM +0100, Pavel Březina wrote:
>>>>I see a number of compilation warnings with the patches (using
clang-3.3)
>>>>
>>>>/home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:9:
warning: variable 'in_transaction' is used uninitialized whenever 'if'
condition is true [-Wsometimes-uninitialized]
>>>> if (ret != EOK) {
>>>> ^~~~~~~~~~
>>>>/home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:9:
note: uninitialized use occurs here
>>>> if (in_transaction) {
>>>> ^~~~~~~~~~~~~~
>>>>/home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:5:
note: remove the 'if' if its condition is always false
>>>> if (ret != EOK) {
>>>> ^~~~~~~~~~~~~~~~~
>>>>/home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:778:9:
warning: variable 'in_transaction' is used uninitialized whenever 'if'
condition is true [-Wsometimes-uninitialized]
>>>> if (groups == NULL) {
>>>> ^~~~~~~~~~~~~~
>>>
>>>I also seen following warning with gcc and higher optimization level:
>>>/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:
>>>In function 'sdap_ad_tokengroups_initgr_posix_tg_done':
>>>/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:1008:20:
>>>warning: 'num_sids' may be used uninitialized in this function
>>>[-Wmaybe-uninitialized]
>>>/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:
>>>In function 'sdap_ad_tokengroups_initgr_mapping_done':
>>>/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:8:
>>>warning: 'in_transaction' may be used uninitialized in this function
>>>[-Wuninitialized]
>>>/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:777:14:
>>>warning: 'num_sids' may be used uninitialized in this function
>>>[-Wmaybe-uninitialized]
>>>
>>>New patches attached. Here's the diff.
>>>
>>>--- a/src/providers/ldap/sdap_async_initgroups_ad.c
>>>+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
>>>@@ -746,14 +746,14 @@ static void
>>>sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
>>> const char *name = NULL;
>>> const char *sid = NULL;
>>> char **sids = NULL;
>>>- size_t num_sids;
>>>+ size_t num_sids = 0;
>>> size_t i;
>>> time_t now;
>>> gid_t gid;
>>> char **groups = NULL;
>>> size_t num_groups;
>>> errno_t ret, sret;
>>>- bool in_transaction;
>>>+ bool in_transaction = false;
>>>
>>> tmp_ctx = talloc_new(NULL);
>>> if (tmp_ctx == NULL) {
>>>@@ -977,7 +977,7 @@ sdap_ad_tokengroups_initgr_posix_tg_done(struct
>>>tevent_req *subreq)
>>> const char *name = NULL;
>>> char *sid = NULL;
>>> char **sids = NULL;
>>>- size_t num_sids;
>>>+ size_t num_sids = 0;
>>> char **valid_groups = NULL;
>>> size_t num_valid_groups;
>>> char **missing_sids = NULL;
>>>
>>>
>>
>>Thank you, I think these patches are fine so ACK from me. But I would
>>like to run them through Coverity, so I'll push them when the scan
>>finishes.
>>
>>Thanks for the big patches!
>
>Hi Pavel,
>sorry but there are some Coverity warnings with the latest iteration of
>patches. The short version is:
>
>Error: UNINIT (CWE-457): [#def1]
>sssd-1.11.90/src/providers/ldap/sdap_async_initgroups_ad.c:756:
>var_decl: Declaring variable "in_transaction" without initializer.
>sssd-1.11.90/src/providers/ldap/sdap_async_initgroups_ad.c:873:
>uninit_use: Using uninitialized value "in_transaction".
>
>Error: COMPILER_WARNING: [#def2]
>sssd-1.11.90/src/providers/ldap/sdap_async_initgroups_ad.c: scope_hint:
>In function 'sdap_ad_tokengroups_initgr_mapping_done'
>sssd-1.11.90/src/providers/ldap/sdap_async_initgroups_ad.c:873:8:
>warning: 'in_transaction' may be used uninitialized in this function
>[-Wmaybe-uninitialized]
># if (in_transaction) {
># ^
>
>Error: FORWARD_NULL (CWE-476): [#def3]
>sssd-1.11.90/src/providers/ldap/sdap_async_initgroups_ad.c:741:
>assign_zero: Assigning: "state" = "NULL".
>sssd-1.11.90/src/providers/ldap/sdap_async_initgroups_ad.c:874:
>var_deref_op: Dereferencing null pointer "state".
>
>I'll send you the e-mail with the full report (sorry, can't paste it
>here, the Coverity instance is behind RH firewall).
>
Hi,
are you sure you applied the latest patches? Because these issues
were fixed there.
No, I'm not actually because I no longer have the SRPM I tested handy.
But I'm about to put my latest patches through Coverity so I'll run your
as well.