Bug 690584 - fix coverity resource leak issues
Summary: fix coverity resource leak issues
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Directory Server
Version: 1.2.8
Hardware: Unspecified
OS: Unspecified
low
low
Target Milestone: ---
Assignee: Rich Megginson
QA Contact: Chandrasekar Kannan
URL:
Whiteboard:
Depends On:
Blocks: 639035 389_1.2.9
TreeView+ depends on / blocked
 
Reported: 2011-03-24 17:34 UTC by Rich Megginson
Modified: 2015-01-04 23:47 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-17 14:09:01 UTC
Embargoed:


Attachments (Terms of Use)
0001-Bug-690584-10691-ldbm_back_init-fix-coverity-resourc.patch (3.22 KB, patch)
2011-03-25 16:51 UTC, Rich Megginson
nkinder: review+
Details | Diff
0002-Bug-690584-10690-10689-attrcrypt_get_ssl_cert_name-f.patch (1.57 KB, patch)
2011-03-25 16:52 UTC, Rich Megginson
nkinder: review+
Details | Diff
0003-Bug-690584-10688-dblayer_make_env-fix-coverity-resou.patch (3.02 KB, patch)
2011-03-25 16:53 UTC, Rich Megginson
nkinder: review+
Details | Diff
0004-Bug-690584-10669-10668-cl5ImportLDIF-fix-coverity-re.patch (5.51 KB, patch)
2011-03-25 16:53 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0005-Bug-690584-10658-linked_attrs_pre_op-fix-coverity-re.patch (2.24 KB, patch)
2011-03-25 16:54 UTC, Rich Megginson
nkinder: review+
Details | Diff
0006-Bug-690584-10655-acllas__handle_group_entry-fix-cove.patch (1.79 KB, patch)
2011-03-25 16:54 UTC, Rich Megginson
nkinder: review+
Details | Diff
0007-Bug-690584-10654-10653-str2entry_dupcheck-fix-coveri.patch (2.12 KB, patch)
2011-03-25 16:55 UTC, Rich Megginson
nkinder: review+
Details | Diff
0008-Bug-690584-10652-10651-10650-10649-10648-10647-send_.patch (3.23 KB, patch)
2011-03-25 17:01 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0009-Bug-690584-10643-hash_rootpw-fix-coverity-resource-l.patch (1.24 KB, patch)
2011-03-25 17:02 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0010-Bug-690584-10641-reslimit_bv2int-fix-coverity-resour.patch (2.83 KB, patch)
2011-03-25 17:03 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0011-Bug-690584-10691-ldbm_back_init-fix-coverity-resourc.patch (1.05 KB, patch)
2011-03-28 21:12 UTC, Rich Megginson
nkinder: review+
Details | Diff
0012-Bug-690584-10652-10651-10650-10649-10648-10647-send_.patch (1.80 KB, patch)
2011-03-28 21:13 UTC, Rich Megginson
nkinder: review+
Details | Diff

Description Rich Megginson 2011-03-24 17:34:23 UTC
#10691: RESOURCE_LEAK
ldbm_back_init()
 107        /* allocate backend-specific stuff */
Calling allocation function "slapi_ch_calloc". [show details]
Assigning: "li" = storage returned from "slapi_ch_calloc(1UL, sizeof (struct ldbminfo) /*248*/)".
 108        li = (struct ldbminfo *) slapi_ch_calloc( 1, sizeof(struct ldbminfo) );
...
 124        if (dblayer_init(li)) {
At conditional (2): "slapd_ldap_debug & 0x4000" taking the true branch.
 125                LDAPDebug( LDAP_DEBUG_ANY, "ldbm_back_init: dblayer_init failed\n",0, 0, 0 );
Variable "li" going out of scope leaks the storage it points to.
 126                return (-1);

Comment 1 Rich Megginson 2011-03-24 17:36:39 UTC
10690: RESOURCE_LEAK
attrcrypt_get_ssl_cert_name
Calling allocation function "slapi_entry_attr_get_charptr". [show details]
Assigning: "personality" = storage returned from "slapi_entry_attr_get_charptr(config_entry, "nssslpersonalityssl")".
 158        personality = 
 159                slapi_entry_attr_get_charptr( config_entry, "nssslpersonalityssl" );
At conditional (1): "token" taking the false branch.
 160        if (token && personality) {
...
Variable "personality" going out of scope leaks the storage it points to.
 172        return 0;

Comment 2 Rich Megginson 2011-03-24 17:38:00 UTC
10689: RESOURCE_LEAK
Calling allocation function "slapi_entry_attr_get_charptr". [show details]
Assigning: "token" = storage returned from "slapi_entry_attr_get_charptr(config_entry, "nsssltoken")".
 157        token = slapi_entry_attr_get_charptr( config_entry, "nsssltoken" );
...
At conditional (1): "token" taking the true branch.
At conditional (2): "personality" taking the false branch.
 160        if (token && personality) {
Variable "token" going out of scope leaks the storage it points to.
 172        return 0;

Comment 3 Rich Megginson 2011-03-24 17:39:47 UTC
10688: RESOURCE_LEAK
dblayer_make_env
Calling allocation function "slapi_ch_calloc". [show details]
Assigning: "pEnv" = storage returned from "slapi_ch_calloc(1UL, sizeof (dblayer_private_env) /*24*/)".
1196    pEnv = (struct dblayer_private_env *)slapi_ch_calloc(1,
1197                                                   sizeof(dblayer_private_env));
...
At conditional (17): "pEnv->dblayer_env_lock" taking the false branch.
1253    if (pEnv->dblayer_env_lock) {
1254        *env = pEnv;
1255    } else {
At conditional (18): "slapd_ldap_debug & 0x4000" taking the true branch.
1256        LDAPDebug(LDAP_DEBUG_ANY,
1257            "ERROR -- Failed to create RWLock (returned: %d).\n", 
1258            ret, 0, 0);
1259    }
1260
Variable "pEnv" going out of scope leaks the storage it points to.
1261    return ret;

Comment 4 Rich Megginson 2011-03-24 17:46:31 UTC
10669: RESOURCE_LEAK
cl5ImportLDIF

make sure maxvals is freed if dbfile == NULL

Comment 5 Rich Megginson 2011-03-24 17:47:17 UTC
10668: RESOURCE_LEAK
cl5ImportLDIF

make sure purgevals is freed if dbfile == NULL

Comment 6 Rich Megginson 2011-03-24 19:51:19 UTC
10658: RESOURCE_LEAK
linked_attrs_pre_op
just remove smods and get rid of the code that uses it

Comment 7 Rich Megginson 2011-03-24 19:52:47 UTC
10655: RESOURCE_LEAK
acllas__handle_group_entry
Calling allocation function "slapi_create_dn_string". [show details]
Assigning: "n_dn" = storage returned from "slapi_create_dn_string(attrVal->bv_val)".
2407                                n_dn = slapi_create_dn_string( attrVal->bv_val );
At conditional (1): "NULL == n_dn" taking the false branch.
2408                                if (NULL == n_dn) {
2409                                        slapi_log_error( SLAPI_LOG_FATAL, plugin_name,
2410                                                "acllas__handle_group_entry: Invalid syntax: %s\n",
2411                                                attrVal->bv_val );
2412                                        return 0;
2413                                }
2414                                n = ++info->lu_idx;
At conditional (2): "n < 0" taking the true branch.
2415                                if (n < 0) {
2416                                        slapi_log_error( SLAPI_LOG_FATAL, plugin_name,
2417                                                  "acllas__handle_group_entry: last member index lu_idx is overflown:%d: Too many group ACL members\n", n);
Variable "n_dn" going out of scope leaks the storage it points to.
2418                                        return 0;

Comment 8 Rich Megginson 2011-03-24 19:56:09 UTC
10654: RESOURCE_LEAK
str2entry_dupcheck
Calling allocation function "str2entry_state_information_from_type" on "maxcsn". [show details]
 786                str2entry_state_information_from_type(type,&valuecsnset,&attributedeletioncsn,&maxcsn,&value_state,&attr_state);
...
Variable "maxcsn" going out of scope leaks the storage it points to.
 812                                        return NULL;

Comment 9 Rich Megginson 2011-03-24 19:58:19 UTC
10653: RESOURCE_LEAK
str2entry_dupcheck
Calling allocation function "str2entry_state_information_from_type" on "attributedeletioncsn". [show details]
 786                str2entry_state_information_from_type(type,&valuecsnset,&attributedeletioncsn,&maxcsn,&value_state,&attr_state);
...
Variable "attributedeletioncsn" going out of scope leaks the storage it points to.
 882                        continue;

Comment 10 Rich Megginson 2011-03-24 20:05:05 UTC
10652: RESOURCE_LEAK
send_specific_attrs
Calling allocation function "slapi_vattr_namespace_values_get_sp" on "values". [show details]
1108                        rc = slapi_vattr_namespace_values_get_sp(
...
1157                                if ( rc != 0 ) {
Variable "values" going out of scope leaks the storage it points to.
1158                                                goto exit;

Comment 11 Rich Megginson 2011-03-24 20:07:20 UTC
10650: RESOURCE_LEAK
send_specific_attrs
Calling allocation function "slapi_vattr_namespace_values_get_sp" on "type_name_disposition".
[show details]
1108                        rc = slapi_vattr_namespace_values_get_sp(
...
1157                                if ( rc != 0 ) {
Variable "type_name_disposition" going out of scope leaks the storage it points to.
1158                                                goto exit;

Comment 12 Rich Megginson 2011-03-24 20:12:57 UTC
10649: RESOURCE_LEAK
send_all_attrs
Calling allocation function "slapi_vattr_namespace_values_get_sp" on "values". [show details]
 982                        rc = slapi_vattr_namespace_values_get_sp(
...
Variable "values" going out of scope leaks the storage it points to.
1032                                                goto exit;
...
Variable "values" going out of scope leaks the storage it points to.
1044        }

Comment 13 Rich Megginson 2011-03-24 20:13:35 UTC
10648: RESOURCE_LEAK
send_all_attrs
Calling allocation function "slapi_vattr_namespace_values_get_sp" on "actual_type_name".
[show details]
 982                        rc = slapi_vattr_namespace_values_get_sp(
...
Variable "actual_type_name" going out of scope leaks the storage it points to.
1032                                                goto exit;
...
Variable "actual_type_name" going out of scope leaks the storage it points to.
1044        }

Comment 14 Rich Megginson 2011-03-24 20:14:08 UTC
10651: RESOURCE_LEAK
send_specific_attrs
Calling allocation function "slapi_vattr_namespace_values_get_sp" on "actual_type_name".
[show details]
1108                        rc = slapi_vattr_namespace_values_get_sp(
...
1157                                if ( rc != 0 ) {
Variable "actual_type_name" going out of scope leaks the storage it points to.
1158                                                goto exit;

Comment 15 Rich Megginson 2011-03-24 20:15:21 UTC
10647: RESOURCE_LEAK
send_all_attrs
Calling allocation function "slapi_vattr_namespace_values_get_sp" on
"type_name_disposition".
[show details]
 982                        rc = slapi_vattr_namespace_values_get_sp(
...
Variable "type_name_disposition" going out of scope leaks the storage it points to.
1032                                                goto exit;
...
Variable "type_name_disposition" going out of scope leaks the storage it points to.
1044        }

Comment 16 Rich Megginson 2011-03-24 20:18:00 UTC
10643: RESOURCE_LEAK
hash_rootpw
At conditional (1): "pw_val2scheme(val, NULL, 0)" taking the true branch.
Calling allocation function "pw_val2scheme". [show details]
Failing to save storage allocated by "pw_val2scheme(val, NULL, 0)" leaks it.
1244                        if (pw_val2scheme (val, NULL, 0)) {
1245                                /* Value is pre-hashed, no work to do for this value */
1246                                continue;

Comment 17 Rich Megginson 2011-03-24 20:25:06 UTC
10641: RESOURCE_LEAK
reslimit_bv2int
the comment is wrong - slapi_value_get_int does return a signed int - the fix is to get rid of this function

Comment 18 Rich Megginson 2011-03-25 16:51:42 UTC
Created attachment 487604 [details]
0001-Bug-690584-10691-ldbm_back_init-fix-coverity-resourc.patch

fixes problem in comment c#0

Comment 19 Rich Megginson 2011-03-25 16:52:16 UTC
Created attachment 487605 [details]
0002-Bug-690584-10690-10689-attrcrypt_get_ssl_cert_name-f.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c1

Comment 20 Rich Megginson 2011-03-25 16:53:04 UTC
Created attachment 487606 [details]
0003-Bug-690584-10688-dblayer_make_env-fix-coverity-resou.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c3

Comment 21 Rich Megginson 2011-03-25 16:53:42 UTC
Created attachment 487607 [details]
0004-Bug-690584-10669-10668-cl5ImportLDIF-fix-coverity-re.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c4 and https://bugzilla.redhat.com/show_bug.cgi?id=690584#c5

Comment 22 Rich Megginson 2011-03-25 16:54:11 UTC
Created attachment 487608 [details]
0005-Bug-690584-10658-linked_attrs_pre_op-fix-coverity-re.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c6

Comment 23 Rich Megginson 2011-03-25 16:54:44 UTC
Created attachment 487609 [details]
0006-Bug-690584-10655-acllas__handle_group_entry-fix-cove.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c7

Comment 24 Rich Megginson 2011-03-25 16:55:41 UTC
Created attachment 487611 [details]
0007-Bug-690584-10654-10653-str2entry_dupcheck-fix-coveri.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c8 and https://bugzilla.redhat.com/show_bug.cgi?id=690584#c9

Comment 26 Rich Megginson 2011-03-25 17:02:26 UTC
Created attachment 487613 [details]
0009-Bug-690584-10643-hash_rootpw-fix-coverity-resource-l.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c16

Comment 27 Rich Megginson 2011-03-25 17:03:08 UTC
Created attachment 487616 [details]
0010-Bug-690584-10641-reslimit_bv2int-fix-coverity-resour.patch

fixes https://bugzilla.redhat.com/show_bug.cgi?id=690584#c17

Comment 28 Noriko Hosoi 2011-03-25 17:55:14 UTC
Comment on attachment 487607 [details]
0004-Bug-690584-10669-10668-cl5ImportLDIF-fix-coverity-re.patch

Bug Description: #10669 #10668 cl5ImportLDIF - fix coverity resource leak issues

Due to the nature of the changelog, RUVs appear at the beginning of the log as one section. So, if (rc != CL5_SUCCESS) clause is executed once in the outer while loop.  (I ran valgrind and saw no leaks.)  But the proposed fix looks safer and works correctly if the assumption is changed.  Thanks!!

Comment 29 Rich Megginson 2011-03-25 19:35:35 UTC
(In reply to comment #28)
> Comment on attachment 487607 [details]
> 0004-Bug-690584-10669-10668-cl5ImportLDIF-fix-coverity-re.patch
> 
> Bug Description: #10669 #10668 cl5ImportLDIF - fix coverity resource leak
> issues
> 
> Due to the nature of the changelog, RUVs appear at the beginning of the log as
> one section. So, if (rc != CL5_SUCCESS) clause is executed once in the outer
> while loop.  (I ran valgrind and saw no leaks.)  But the proposed fix looks
> safer and works correctly if the assumption is changed.  Thanks!!

Ok.  Good to know it is not a problem in current code.  Thanks for verifying that.

Comment 30 Rich Megginson 2011-03-25 21:19:13 UTC
To ssh://git.fedorahosted.org/git/389/ds.git
   5b6d116..6c98225  master -> master
commit 6c9822577b32af674dae2f884030bf2fd8a51bc2
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 10:48:27 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: The comment was incorrect - slapi_value_get_int does handle
    signed values correctly - it uses the same function atoi - the fix is to
    get rid of reslimit_bv2int
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 8a88106ba1d49818ab36548303d16a156e921afd
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 10:40:30 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: store the pointer returned by pw_val2scheme and free it
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 3e97d377e0899793fe156d09178a999952c792bf
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 10:36:09 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: if there is an error, we still have to free the values allo
    by slapi_vattr_namespace_values_get_sp() - so just continue the for loop, bu
    rc != 0 just free the remaining values and continue
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 51bf2d258d097264d1d8c0d344ee40cbca09cc9d
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 09:58:33 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: make sure to free maxcsn and attributedeletioncsn before th
    out of scope
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit f25fef365df535c154a3cbffbda7317c5766fb38
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 09:41:30 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: Make sure n_dn is freed before returning.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 5bbe379e3753b397a13fb0f3a418c90a717a1d7a
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 09:36:47 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: just get rid of smods - not used here
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit b1cebedd1b2de30f7dd2885d57eaec19d445044a
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 09:29:23 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: I didn't quite follow the logic, but it looked as though
    maxvals and purgevals could have been re-malloced if the code allocated
    them and went through the loop more than once.  So I changed the code to
    just realloc as needed (realloc NULL is the same as malloc).  When building
    maxvals and purgevals, always NULL terminate the list so we don't have to
    do that later.  Finally, I moved the code that cleaned up maxvals and
    purgevals to the end of the function so that they will always be called.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 909150899c5262b64af5b5a4b6935a98f8593ece
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 08:52:32 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: do not return upon failure, goto fail instead.  The fail
    condition will clean up any memory allocated in the function.  I had to
    move dblayer_free_env to before dblayer_make_env.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit e99c735d37b2cec59d46bd270f661b13385cfcdd
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 08:27:52 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: always free token and personality at the end of the functio
    - set personality to NULL if the memory was passed off to cert_name.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 5b44581e0bee31cd455bbfe0f7cb109e7e7b25f8
Author: Rich Megginson <rmeggins>
Date:   Fri Mar 25 08:17:38 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: do not return (or exit!) from ldbm_back_init() - just goto
    fail and clean up the ldbm_instance upon error.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no

Comment 31 Rich Megginson 2011-03-28 21:12:44 UTC
Created attachment 488272 [details]
0011-Bug-690584-10691-ldbm_back_init-fix-coverity-resourc.patch

Comment 32 Rich Megginson 2011-03-28 21:13:06 UTC
Created attachment 488273 [details]
0012-Bug-690584-10652-10651-10650-10649-10648-10647-send_.patch

Comment 33 Rich Megginson 2011-03-28 21:33:06 UTC
To ssh://git.fedorahosted.org/git/389/ds.git
   67e2651..d91cd63  master -> master
commit d91cd6344ca40f92c19856159add2a08d4f78993
Author: Rich Megginson <rmeggins>
Date:   Mon Mar 28 15:09:35 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: if it is possible for slapi_vattr_namespace_values_get_sp
w
    item_count == 0, make sure to free the values allocated
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 9ea3531355c40b9e9742ca11254e090394db56dd
Author: Rich Megginson <rmeggins>
Date:   Mon Mar 28 15:01:14 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: also have to free li and set the pblock pointer to NULL
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no


Note You need to log in before you can comment on or make changes to this bug.