#47740 Coverity issue in 1.3.3
Closed: Fixed None Opened 6 years ago by mreynolds.

Investigate the 44 issues found by the new version of coverity


Replying to [comment:1 mreynolds]:

Attachment 0001-Ticket-47740-Coverity-Fixes-Mark-part-1.patch​ added
Mark - part 1
ack!

(Mark - part 1)

git merge coverity
Updating 1e00913..d3ed247
Fast-forward
ldap/servers/plugins/chainingdb/cb_bind.c | 5 +-
ldap/servers/plugins/chainingdb/cb_config.c | 70 +++++++++-----------
ldap/servers/plugins/chainingdb/cb_instance.c | 61 ++++++++---------
ldap/servers/plugins/posix-winsync/posix-winsync.c | 9 ++-
.../plugins/replication/repl5_replica_config.c | 2 +
lib/ldaputil/certmap.c | 5 +-
6 files changed, 75 insertions(+), 77 deletions(-)

git push origin master
1e00913..d3ed247 master -> master

commit d3ed247
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Mar 11 16:44:34 2014 -0400

ba9248d..836ee43 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

450607a..e592de0 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

e118eab..53d7326 389-ds-base-1.3.0 -> 389-ds-base-1.3.0

9760c04..ea59c06 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

you always free uniqueid, but it is either assigned uuid and should persist or should be freed in the various cases in the switch-if-else statements

Replying to [comment:4 lkrispen]:

you always free uniqueid, but it is either assigned uuid and should persist or should be freed in the various cases in the switch-if-else statements

Good catch! But there are still some places where it needed to be freed. New patch is attached.

Attachment 0001-Ticket-47740-Fix-sync-plugin-resource-leaks.patch​ added
Mark - part 2 - sync plugin leak fixes (revised)

ack.

(Mark - part 2)

git merge coverity
Updating d3ed247..78f1bda
Fast-forward
ldap/servers/plugins/sync/sync_refresh.c | 25 ++++++++++++-------------
1 files changed, 12 insertions(+), 13 deletions(-)

git push origin master
d3ed247..78f1bda master -> master

commit 78f1bda
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Mar 12 11:19:31 2014 -0400

5b4ca6d..549b981 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

(Mark - part 3)

git merge coverity
Updating 78f1bda..06919f5
Fast-forward
ldap/servers/plugins/replication/legacy_consumer.c | 10 ++--
ldap/servers/slapd/back-ldbm/import-threads.c | 1 +
ldap/servers/slapd/mapping_tree.c | 32 ++++++++---
ldap/servers/slapd/passwd_extop.c | 4 +-
ldap/servers/slapd/tools/mmldif.c | 2 +
ldap/servers/slapd/tools/pwenc.c | 4 +-
ldap/servers/slapd/tools/rsearch/sdattable.c | 58 +++++++++++++-------
7 files changed, 73 insertions(+), 38 deletions(-)

git push origin master
78f1bda..06919f5 master -> master

commit 06919f5
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Mar 12 14:38:50 2014 -0400

549b981..9eecbeb 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

6f90450..ff78177 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

d10e5e3..2b9788b 389-ds-base-1.3.0 -> 389-ds-base-1.3.0

dae03f0..fd8668b 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

Attachment 0001-Ticket-47740-Fix-coverity-erorrs-Part-4.patch​ added
Mark - part 4

ack. (bummer! proxyauth.c is not leaking... ;)

Replying to [comment:11 nhosoi]:

Attachment 0001-Ticket-47740-Fix-coverity-erorrs-Part-4.patch​ added
Mark - part 4

ack. (bummer! proxyauth.c is not leaking... ;)

There is another potential leak in acleffectiverights.c that is proxy auth related, but I think it's a false positive. I need to run some valgrind tests tomorrow.

(Mark - part 4)

git merge coverity
Updating 06919f5..48c1f2a
Fast-forward
ldap/servers/plugins/acl/aclplugin.c | 8 ++----
ldap/servers/plugins/chainingdb/cb_controls.c | 14 ++++++-----
ldap/servers/plugins/chainingdb/cb_instance.c | 18 +++++++-------
ldap/servers/plugins/replication/repl5_replica.c | 3 +-
.../plugins/replication/repl5_replica_config.c | 13 +++++++---
ldap/servers/slapd/back-ldbm/upgrade.c | 3 ++
ldap/servers/slapd/main.c | 5 ++-
ldap/servers/slapd/proxyauth.c | 24 ++++++++++----------
8 files changed, 49 insertions(+), 39 deletions(-)

git push origin master
06919f5..48c1f2a master -> master

commit 48c1f2a
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Mar 12 16:58:32 2014 -040

9eecbeb..c5c4ecd 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

ff78177..665a493 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

2b9788b..9df19f8 389-ds-base-1.3.0 -> 389-ds-base-1.3.0

fd8668b..85d062b 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

You don't need to check for NULL before calling free:
{{{

532 if (dncomps && *dncomps) free(dncomps);
532 if (dncomps) free(dncomps);
}}}
can be just
{{{
free(dncomps); dncomps = NULL;
}}}
Otherwise, ack

git merge coverity
Updating 48c1f2a..b9901f1
Fast-forward
ldap/servers/plugins/acl/acleffectiverights.c | 1 +
ldap/servers/plugins/replication/urp.c | 1 +
ldap/servers/slapd/saslbind.c | 1 +
lib/ldaputil/certmap.c | 162 ++++++++++++-------------
4 files changed, 82 insertions(+), 83 deletions(-)

git push origin master
48c1f2a..b9901f1 master -> master

commit b9901f1
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Mar 13 14:07:49 2014 -0400

c5c4ecd..eb10369 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

665a493..0a28515 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

9df19f8..6886502 389-ds-base-1.3.0 -> 389-ds-base-1.3.0

85d062b..1c552d5 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

{{{
464 free(instancename);
465 instancename = NULL;
464 if(instancename){
465 free(instancename);
466 instancename = NULL;
467 }
}}}
The current version should be correct - you do not have to check for NULL before free. Not sure what coverity is complaining about.

Replying to [comment:17 rmeggins]:

{{{
464 free(instancename);
465 instancename = NULL;
464 if(instancename){
465 free(instancename);
466 instancename = NULL;
467 }
}}}
The current version should be correct - you do not have to check for NULL before free. Not sure what coverity is complaining about.

Ok, I adjusted it, new patch attached...

Pushed 0001-Ticket-47740-Coverity-issue-in-1.3.3.patch​
Master:
b9901f1..a7ac181 master -> master
commit 6f51e06

389-ds-base-1.3.2:
4ffa824..f3afc11 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit f3afc11

389-ds-base-1.3.1:
0a28515..093a146 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit cfb821c

389-ds-base-1.2.11:
1c552d5..2bb0f1b 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit d8395ec

Mark - part 6

git merge coverity
Updating 71a120d..d398078
Fast-forward
ldap/servers/plugins/memberof/memberof.c | 2 +-
ldap/servers/slapd/tools/migratecred.c | 2 +-
ldap/servers/snmp/main.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)

git push origin master
71a120d..d398078 master -> master

commit d398078
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Mar 13 15:43:28 2014 -0400

e21aada..24ea155 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

0b5894f..9a91525 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

2bb0f1b..0d6db17 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

(Fix patch 5)

git merge coverity
Updating 572ae72..33b98f1
Fast-forward
lib/ldaputil/certmap.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

git push origin master
572ae72..33b98f1 master -> master

commit 33b98f1
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Mar 14 12:33:40 2014 -0400

bc61ed7..7b464ad 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

9a91525..c2f1be4 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

6886502..09e70fc 389-ds-base-1.3.0 -> 389-ds-base-1.3.0

0d6db17..b46e1fd 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

This looks to me we are adding a chance to free(NULL). If subjectDN == NULL, you call free(subjectDN)... (note: this is not slapi_ch_free :)
{{{
diff --git a/lib/ldaputil/certmap.c b/lib/ldaputil/certmap.c index 0935e4d..b4267dd 100644
a b static int ldapu_cert_searchfn_default (void cert, LDAP ld,
737 737
738 738 rv = ldapu_get_cert_subject_dn(cert, &subjectDN);
739 739
740 if (rv != LDAPU_SUCCESS || !subjectDN || !subjectDN) return rv;
740 if (rv != LDAPU_SUCCESS || !subjectDN || !
subjectDN){
741 free(subjectDN);
742 return rv;
743 }
}}}

Replying to [comment:28 nhosoi]:

This looks to me we are adding a chance to free(NULL). If subjectDN == NULL, you call free(subjectDN)... (note: this is not slapi_ch_free :)
{{{
diff --git a/lib/ldaputil/certmap.c b/lib/ldaputil/certmap.c index 0935e4d..b4267dd 100644
a b static int ldapu_cert_searchfn_default (void cert, LDAP ld,
737 737
738 738 rv = ldapu_get_cert_subject_dn(cert, &subjectDN);
739 739
740 if (rv != LDAPU_SUCCESS || !subjectDN || !subjectDN) return rv;
740 if (rv != LDAPU_SUCCESS || !subjectDN || !
subjectDN){
741 free(subjectDN);
742 return rv;
743 }
}}}

New patch attached...

Replying to [comment:28 nhosoi]:

This looks to me we are adding a chance to free(NULL). If subjectDN == NULL, you call free(subjectDN)... (note: this is not slapi_ch_free :)

That's ok. free(NULL) has been a no-op for many years now e.g. man malloc:
{{{
free() frees the memory space pointed to by ptr, which must have been
returned by a previous call to malloc(), calloc() or realloc(). Other-
wise, or if free(ptr) has already been called before, undefined behav-
ior occurs. If ptr is NULL, no operation is performed.
}}}

{{{
diff --git a/lib/ldaputil/certmap.c b/lib/ldaputil/certmap.c index 0935e4d..b4267dd 100644
a b static int ldapu_cert_searchfn_default (void cert, LDAP ld,
737 737
738 738 rv = ldapu_get_cert_subject_dn(cert, &subjectDN);
739 739
740 if (rv != LDAPU_SUCCESS || !subjectDN || !subjectDN) return rv;
740 if (rv != LDAPU_SUCCESS || !subjectDN || !
subjectDN){
741 free(subjectDN);
742 return rv;
743 }
}}}

Thanks, Rich! I was looking at the same man page. :) My comment has conflicted with yours...

comment:14 says

You don't need to check for NULL before calling free:
{{{
532 if (dncomps && *dncomps) free(dncomps);
532 if (dncomps) free(dncomps);
}}}
can be just
free(dncomps); dncomps = NULL;

All right. "man free" says free(NULL) does nothing. So, your previous patch was good in terms of free(subjectDN). Regarding subjectDN == "" case, your new patch does the right thing. So, your second patch 0001-Ticket-47740-Fix-coverity-issues-part-7.patch​ has my double ack. :)
The free() function frees the memory space pointed to by ptr, which must have been returned by a pre-
vious call to malloc(), calloc() or realloc(). Otherwise, or if free(ptr) has already been called
before, undefined behavior occurs. If ptr is NULL, no operation is performed.

git merge coverity
Updating 4fc53e1..afadb40
Fast-forward
ldap/servers/plugins/chainingdb/cb_bind.c | 43 ++---
ldap/servers/plugins/chainingdb/cb_config.c | 175 +++++++++++---------
ldap/servers/plugins/chainingdb/cb_instance.c | 2 +-
ldap/servers/plugins/mep/mep.c | 1 +
ldap/servers/plugins/posix-winsync/posix-winsync.c | 13 +-
ldap/servers/plugins/sync/sync_refresh.c | 2 +
ldap/servers/slapd/mapping_tree.c | 13 +-
lib/ldaputil/certmap.c | 4 +-
8 files changed, 136 insertions(+), 117 deletions(-)

git push origin master
4fc53e1..afadb40 master -> master

commit afadb40
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Mar 24 19:05:36 2014 -0700

f8f063c..7a50bc6 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

feed9ba..99609ce 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

fd954a7..b8297ad 389-ds-base-1.3.0 -> 389-ds-base-1.3.0

feed9ba..99609ce 389-ds-base-1.3.1 -> 389-ds-base-1.3.1

Metadata Update from @mreynolds:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.2.11.29

3 years ago

Login to comment on this ticket.

Metadata