Investigate the 44 issues found by the new version of coverity
Mark - part 1 0001-Ticket-47740-Coverity-Fixes-Mark-part-1.patch
Replying to [comment:1 mreynolds]:
Attachment 0001-Ticket-47740-Coverity-Fixes-Mark-part-1.patch added Mark - part 1 ack!
Coverity 11778 - Resource leak 0001-Ticket-47740-Coverity-issue-in-1.3.3.patch
(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
Mark - part 2 - sync plugin leak fixes (revised) 0001-Ticket-47740-Fix-sync-plugin-resource-leaks.patch
Replying to [comment:4 lkrispen]:
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 0001-Ticket-47640-Fix-coverity-issues-part-3.patch
(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
Mark - part 4 0001-Ticket-47740-Fix-coverity-erorrs-Part-4.patch
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
Mark - part 5 (revised) 0001-Ticket-47740-Fix-coverity-issues-Part-5.patch
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]:
Ok, I adjusted it, new patch attached...
Part 6 - revised 0001-Ticket-47740-Fix-coverity-issues-null-deferences-Par.patch
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 crash caused by Part 5 patch 0001-Ticket-47740-Crash-caused-by-changes-to-certmap.c.patch
(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 } }}}
Part 7 (revision) 0001-Ticket-47740-Fix-coverity-issues-part-7.patch
Replying to [comment:28 nhosoi]:
New patch attached...
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
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1109356
Metadata Update from @mreynolds: - Issue assigned to mreynolds - Issue set to the milestone: 1.2.11.29
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1072
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.