Re: [Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-28 Thread Martin Kosek
On 02/27/2013 01:55 PM, Martin Kosek wrote:
> On 02/27/2013 01:39 PM, Martin Kosek wrote:
>> On 02/27/2013 12:35 PM, Sumit Bose wrote:
>>> Hi,
>>>
>>> the attached patches 102-107 fix issues found by Coverity which are
>>> tracked by tickets #3422-#3427 and remove an unused variable
>>> (patch 101).
>>>
>>> bye,
>>> Sumit
>>>
>>
>> I see just one issue. In patch 0105:
>>
>> -global_ipactx = (struct ipa_context *)malloc(sizeof(global_ipactx));
>> +global_ipactx = (struct ipa_context *)malloc(sizeof(*global_ipactx));
>>
>> I do not think this will work right. *global_ipactx will just de-reference
>> global_ipactxt and run sizeof on the result, right?
>>
>> I would prefer this change:
>>
>> global_ipactx = (struct ipa_context *)malloc(sizeof(struct ipa_context *));
>>
>> Martin
>>
> 
> We just discussed this in Brno - in fact your change should be right as you
> allocate memory for the whole structure and not just a pointer to the 
> structure
> (though I personally would prefer sizeof(struct ipa_context) as it is 
> clearer).
> 
> Hmrph, C language 101 revisited :-)
> 
> Martin
> 

Good, my tests were OK with your patchset.

ACK for all patches, pushed to master, ipa-3-1.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-27 Thread Alexander Bokovoy

On Wed, 27 Feb 2013, Martin Kosek wrote:

On 02/27/2013 12:35 PM, Sumit Bose wrote:

Hi,

the attached patches 102-107 fix issues found by Coverity which are
tracked by tickets #3422-#3427 and remove an unused variable
(patch 101).

bye,
Sumit



I see just one issue. In patch 0105:

-global_ipactx = (struct ipa_context *)malloc(sizeof(global_ipactx));
+global_ipactx = (struct ipa_context *)malloc(sizeof(*global_ipactx));

I do not think this will work right. *global_ipactx will just de-reference
global_ipactxt and run sizeof on the result, right?

No, sizeof() is compile-time operator. Compiler will do proper type
inferring. See C11 6.5.3.4 for details.

It is a common idiom to do 
   foo = (foo_t*) malloc(sizeof(*foo));



--
/ Alexander Bokovoy

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-27 Thread Martin Kosek
On 02/27/2013 01:39 PM, Martin Kosek wrote:
> On 02/27/2013 12:35 PM, Sumit Bose wrote:
>> Hi,
>>
>> the attached patches 102-107 fix issues found by Coverity which are
>> tracked by tickets #3422-#3427 and remove an unused variable
>> (patch 101).
>>
>> bye,
>> Sumit
>>
> 
> I see just one issue. In patch 0105:
> 
> -global_ipactx = (struct ipa_context *)malloc(sizeof(global_ipactx));
> +global_ipactx = (struct ipa_context *)malloc(sizeof(*global_ipactx));
> 
> I do not think this will work right. *global_ipactx will just de-reference
> global_ipactxt and run sizeof on the result, right?
> 
> I would prefer this change:
> 
> global_ipactx = (struct ipa_context *)malloc(sizeof(struct ipa_context *));
> 
> Martin
> 

We just discussed this in Brno - in fact your change should be right as you
allocate memory for the whole structure and not just a pointer to the structure
(though I personally would prefer sizeof(struct ipa_context) as it is clearer).

Hmrph, C language 101 revisited :-)

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-27 Thread Martin Kosek
On 02/27/2013 12:35 PM, Sumit Bose wrote:
> Hi,
> 
> the attached patches 102-107 fix issues found by Coverity which are
> tracked by tickets #3422-#3427 and remove an unused variable
> (patch 101).
> 
> bye,
> Sumit
> 

I see just one issue. In patch 0105:

-global_ipactx = (struct ipa_context *)malloc(sizeof(global_ipactx));
+global_ipactx = (struct ipa_context *)malloc(sizeof(*global_ipactx));

I do not think this will work right. *global_ipactx will just de-reference
global_ipactxt and run sizeof on the result, right?

I would prefer this change:

global_ipactx = (struct ipa_context *)malloc(sizeof(struct ipa_context *));

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-27 Thread Sumit Bose
Hi,

the attached patches 102-107 fix issues found by Coverity which are
tracked by tickets #3422-#3427 and remove an unused variable
(patch 101).

bye,
Sumit
From 97b3b7dedac28704d51e2fa4b4dc975a20d17ada Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Tue, 19 Feb 2013 12:48:58 +0100
Subject: [PATCH 101/107] ipa-kdb: remove unused variable

---
 daemons/ipa-kdb/ipa_kdb_principals.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index 
46a921cb2f267b977df4a0042d81a49beccb5a91..11c155e64e20d16da98158eb4d7b8034803bf1de
 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -535,7 +535,7 @@ static krb5_error_code ipadb_fetch_principals(struct 
ipadb_context *ipactx,
 krb5_error_code kerr;
 char *src_filter = NULL;
 char *esc_original_princ = NULL;
-int ret, i;
+int ret;
 
 if (!ipactx->lcontext) {
 ret = ipadb_get_connection(ipactx);
-- 
1.8.1.2

From d5a64eced4800190221395d19341918ae417f118 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Tue, 19 Feb 2013 12:49:25 +0100
Subject: [PATCH 102/107] ipa-kdb: Uninitialized scalar variable in
 ipadb_reinit_mspac()

There was a code path where ret was used instead of kerr to save a
return value.

Fixes https://fedorahosted.org/freeipa/ticket/3422
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
c72e762ca4c680def9ed97e492f6edfcc5e0677f..d022bd9a958f2ec591a6a338753b510492b2d400
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1986,12 +1986,11 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context 
*ipactx)
 if (ipactx->mspac && ipactx->mspac->num_trusts == 0) {
 /* Check if there is any trust configured. If not, just return
  * and do not re-initialize the MS-PAC structure. */
-ret = ipadb_mspac_check_trusted_domains(ipactx);
-if (ret == KRB5_KDB_NOENTRY) {
-ret = 0;
+kerr = ipadb_mspac_check_trusted_domains(ipactx);
+if (kerr == KRB5_KDB_NOENTRY) {
+kerr = 0;
 goto done;
-} else if (ret != 0) {
-ret = EIO;
+} else if (kerr != 0) {
 goto done;
 }
 }
-- 
1.8.1.2

From c36fc476b52315012d2d17994d0337761d25cb5e Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Wed, 27 Feb 2013 12:12:45 +0100
Subject: [PATCH 103/107] ipa-sam: Array compared against 0 in
 ipasam_set_trusted_domain()

ipa_mspac_well_known_sids is a globally defined array so the check was
always true.

Fixes https://fedorahosted.org/freeipa/ticket/3423
---
 daemons/ipa-sam/ipa_sam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index 
0d4a27cf6def737412f6ab3ca48df8d1339c15b6..b9fc00c8d08821e09b3242a82b3673a88ace0a51
 100644
--- a/daemons/ipa-sam/ipa_sam.c
+++ b/daemons/ipa-sam/ipa_sam.c
@@ -2293,7 +2293,7 @@ static NTSTATUS ipasam_set_trusted_domain(struct 
pdb_methods *methods,
  &td->trust_forest_trust_info);
}
 
-   for (i = 0; ipa_mspac_well_known_sids && ipa_mspac_well_known_sids[i]; 
i++) {
+   for (i = 0; ipa_mspac_well_known_sids[i]; i++) {
smbldap_make_mod(priv2ld(ldap_state), entry, &mods,
  LDAP_ATTRIBUTE_SID_BLACKLIST_INCOMING,
  ipa_mspac_well_known_sids[i]);
-- 
1.8.1.2

From bffd1ec51ea697887d012ec1eda4171f4affafaf Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Fri, 22 Feb 2013 13:30:30 +0100
Subject: [PATCH 104/107] ipa-kdb: Dereference after null check in
 ipa_kdb_mspac.c

A wrong logic was used to check ipactx.

Fixes https://fedorahosted.org/freeipa/ticket/3424
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
d022bd9a958f2ec591a6a338753b510492b2d400..2c07f4cffe38eb127a7b3f901dc7700e88d6af2a
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1217,7 +1217,7 @@ static krb5_error_code filter_logon_info(krb5_context 
context,
  * */
 if (info->info->info3.sidcount != 0) {
 ipactx = ipadb_get_context(context);
-if (!ipactx && !ipactx->mspac) {
+if (!ipactx || !ipactx->mspac) {
 return KRB5_KDB_DBNOTINITED;
 }
 count = info->info->info3.sidcount;
-- 
1.8.1.2

From e56a47f82f4600ebcb26c012c382c6686125c14a Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Fri, 22 Feb 2013 13:40:08 +0100
Subject: [PATCH 105/107] ipa-lockout: Wrong sizeof argument in ipa_lockout.c

Fixes https://fedorahosted.org/freeipa/ticket/3425
---
 daemons/ipa-slapi-plugins/ipa-lockout/ipa_lockout.c | 2 +-
 1 fil