Bug 609255 - fix coverity Defect Type: Memory - illegal accesses issues
fix coverity Defect Type: Memory - illegal accesses issues
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Directory Server (Show other bugs)
1.2.6
All Linux
medium Severity medium
: ---
: ---
Assigned To: Noriko Hosoi
Chandrasekar Kannan
http://10.16.47.145:8080/defects/inde...
:
Depends On:
Blocks: 389_1.2.7 639035
  Show dependency treegraph
 
Reported: 2010-06-29 14:58 EDT by Noriko Hosoi
Modified: 2015-01-04 18:43 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-05-17 10:09:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
git patch file (9.0) (1.25 KB, patch)
2010-06-29 16:49 EDT, Noriko Hosoi
no flags Details | Diff
git patch file (9.0) (1.03 KB, patch)
2010-06-29 17:36 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.14 KB, patch)
2010-06-29 20:07 EDT, Noriko Hosoi
no flags Details | Diff
git patch file (9.0) (2.72 KB, patch)
2010-06-29 20:47 EDT, Noriko Hosoi
rmeggins: review-
Details | Diff
git patch file (9.0) (1.15 KB, patch)
2010-06-29 20:59 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.12 KB, patch)
2010-06-29 21:07 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.15 KB, patch)
2010-06-29 21:13 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.25 KB, patch)
2010-06-29 21:24 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.16 KB, patch)
2010-06-29 21:32 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.22 KB, patch)
2010-06-30 12:57 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.25 KB, patch)
2010-06-30 13:12 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (2.10 KB, patch)
2010-06-30 13:29 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.62 KB, patch)
2010-06-30 13:46 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.96 KB, patch)
2010-06-30 14:06 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.20 KB, patch)
2010-06-30 14:21 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (1.40 KB, patch)
2010-06-30 20:51 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
git patch file (9.0) (162.59 KB, patch)
2010-07-01 14:41 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff
revised patch (replacing attachment 427830) (2.87 KB, patch)
2010-07-01 17:11 EDT, Noriko Hosoi
rmeggins: review+
Details | Diff

  None (edit)
Description Noriko Hosoi 2010-06-29 14:58:42 EDT
Description of problem:
http://10.16.47.145:8080/defects/index.htm?projectId=10030
click	Memory - illegal accesses
Comment 1 Noriko Hosoi 2010-06-29 16:49:14 EDT
Created attachment 427794 [details]
git patch file (9.0)

12300 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required append_acl_to_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp

Comment:
this code is not used any more. The fix is to have open_file_buf set *buf to NULL after freeing
Comment 2 Noriko Hosoi 2010-06-29 17:10:14 EDT
The patch in comment 1 fixes these defects as well.

12292 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
delete_acl_from_file(char *, char *…) ds/lib/libaccess/acltools.cpp

12294 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
get_acl_from_file(char *, char *, ACLListHandle **…) ds/lib/libaccess/acltools.cpp

12296 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
ACL_FileGetNameList() ds/lib/libaccess/acltools.cpp

12298 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
rename_acl_in_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp
Comment 3 Noriko Hosoi 2010-06-29 17:36:05 EDT
Created attachment 427801 [details]
git patch file (9.0)

12291 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
dbconf_read_default_dbinfo_sub() ds/lib/ldaputil/dbconf.c

Comment:
should never happen - should always break out of the loop at 504 with a valid db_info - but we should set db_info = NULL after line 505 just to make sure
Comment 4 Noriko Hosoi 2010-06-29 20:07:32 EDT
Created attachment 427824 [details]
git patch file (9.0)

12210 UNINIT Triaged Unassigned Bug Minor Fix Required
str2entry_fast() ds/ldap/servers/slapd/entry.c

Comment:
ptype is always NULL the first time through the loop, so a will always be initialized to NULL. But we should explicitly initialize it to NULL in the declaration.
Comment 5 Noriko Hosoi 2010-06-29 20:47:40 EDT
Created attachment 427830 [details]
git patch file (9.0)

12215 UNINIT Triaged Unassigned Bug Minor Fix Required
_cl5LDIF2Operation() ds/ldap/servers/plugins/replication/cl5_api.c

Comment:
should init rawDN to NULL and check if it is NULL before using it

Comment on the particular rawDN at the line 5218:
/* 
 * When it comes here, case T_DNSTR is already
 * passed and rawDN is supposed to set.
 * But it's a good idea to make sure it is
 * not NULL.
 */
Comment 6 Noriko Hosoi 2010-06-29 20:59:49 EDT
Created attachment 427831 [details]
git patch file (9.0)

12216 UNINIT Triaged Unassigned Bug Minor Fix Required
private_protocol_factory() ds/ldap/servers/plugins/replication/repl5_protocol.c

Comment:
should be impossible for type to be anything but one of the valid values, but it wouldn't hurt to init prp to NULL anyway
Comment 7 Noriko Hosoi 2010-06-29 21:07:48 EDT
Created attachment 427832 [details]
git patch file (9.0)

12220 UNINIT Triaged Unassigned Bug Minor Fix Required
create_NSDS50ReplicationExtopPayload() ds/ldap/servers/plugins/replication/repl_extop.c

Comment:
unlikely to cause a problem, but we should init sdn to NULL
Comment 8 Noriko Hosoi 2010-06-29 21:13:04 EDT
Created attachment 427835 [details]
git patch file (9.0)

12221 UNINIT Triaged Unassigned Bug Minor Fix Required
create_NSDS50ReplicationExtopPayload() ds/ldap/servers/plugins/replication/repl_extop.c

Comment:
unlikely to cause a problem, but we should init repl_obj to NULL
Comment 9 Noriko Hosoi 2010-06-29 21:24:53 EDT
Created attachment 427838 [details]
git patch file (9.0)

12222 UNINIT Triaged Unassigned Bug Minor Fix Required
replica_get_purl_for_op() ds/ldap/servers/plugins/replication/repl5_plugins.c

Comment:
In case of an error "cannot obtain consumer connection extension or supplier_ruv", uninitialized purl is returned to the caller. Init purl to NULL.
Comment 10 Noriko Hosoi 2010-06-29 21:32:07 EDT
Created attachment 427839 [details]
git patch file (9.0)

12223 UNINIT Triaged Unassigned Bug Minor Fix Required
my_ber_scanf_attr() ds/ldap/servers/plugins/replication/repl5_total.c

Comment:
In case an error occurs between the line 594 and the line 648,
uninitialized value is passed to slapi_value_free. Need to init 
value to NULL
Comment 11 Noriko Hosoi 2010-06-30 12:57:41 EDT
Created attachment 428039 [details]
git patch file (9.0)

12224 UNINIT Triaged Unassigned Bug Minor Fix Required
windows_private_update_dirsync_control() ds/ldap/servers/plugins/replication/windows_private.c

Comment:
If DIRSYNC control is not found, uninitialized ber is passed to ber_free.
should init ber to NULL.
Comment 12 Noriko Hosoi 2010-06-30 13:12:47 EDT
Created attachment 428048 [details]
git patch file (9.0)

12225 UNINIT Triaged Unassigned Bug Minor Fix Required
windows_private_update_dirsync_control() ds/ldap/servers/plugins/replication/windows_private.c

Comment:
If DIRSYNC control is not found, uninitialized serverCookie is passed to ber_bvfree.  We should init serverCookie to NULL.
Comment 13 Noriko Hosoi 2010-06-30 13:29:12 EDT
Created attachment 428052 [details]
git patch file (9.0)

12230 UNINIT Triaged Unassigned Bug Minor Fix Required
preop_add() ds/ldap/servers/plugins/uiduniq/7bit.c

Comment:
Some cases such as NULL attrName is passed or it does not have a value,
uninitialized "violated" is passed to slapi_ch_smprintf via issue_error.
We should init violated to NULL.

12231 UNINIT Triaged Unassigned Bug Unspecified Fix Required
preop_modify() ds/ldap/servers/plugins/uiduniq/7bit.c

Comment:
Some cases such as NULL attrName is passed or mods were empty,
uninitialized "violated" is passed to slapi_ch_smprintf via issue_error.
We should init violated to NULL.

12232 UNINIT Triaged Unassigned Bug Minor Fix Required
preop_modrdn() ds/ldap/servers/plugins/uiduniq/7bit.c

Comment:
Some cases such as NULL attrName is passed or it does not have a value,
uninitialized "violated" is passed to slapi_ch_smprintf via issue_error.
We should init violated to NULL.
Comment 14 Noriko Hosoi 2010-06-30 13:46:17 EDT
Created attachment 428053 [details]
git patch file (9.0)

12233 UNINIT Triaged Unassigned Bug Minor Fix Required
preop_modify() ds/ldap/servers/plugins/uiduniq/uid.c

Comment:
This is not an issue since attrName is an output variable
for getArguments at the line 689. But to make coverity
happy, we init attrName to NULL.
Comment 15 Noriko Hosoi 2010-06-30 14:06:27 EDT
Created attachment 428057 [details]
git patch file (9.0)

12236 UNINIT Triaged Unassigned Bug Minor Fix Required
dblayer_get_aux_id2entry() ds/ldap/servers/slapd/back-ldbm/dblayer.c

Comment:
If a backend instance info (inst) or ldbminfo (li) or dblayer private 
info (opriv) is not available, uninitialized priv is passed to 
slapi_ch_free_string and slapi_ch_free.  We need to init priv to NULL 
and handle done: case if priv is NULL.
Comment 16 Noriko Hosoi 2010-06-30 14:21:54 EDT
Created attachment 428058 [details]
git patch file (9.0)

12237 UNINIT Triaged Unassigned Bug Minor Fix Required
vlv_trim_candidates_byvalue() ds/ldap/servers/slapd/back-ldbm/vlv.c

Comment:
There is almost no chance to pass uninitialized typedown_value to 
ber_bvecfree unless vlv_request_control value is NULL.  Anyway, we 
init typedown_value to NULL.
Comment 17 Noriko Hosoi 2010-06-30 20:51:51 EDT
Created attachment 428142 [details]
git patch file (9.0)

12210 UNINIT Triaged Unassigned Bug Minor Fix Required
str2entry_fast() ds/ldap/servers/slapd/entry.c

Comment:
ptype is always NULL the first time through the loop, so a will
always be initialized to NULL. But we should explicitly initialize
it to NULL in the declaration.  Moved the declaration of the variable
'a' outside of the loop.  This is needed to initialize the variable
just once at the beginning.
Comment 18 Noriko Hosoi 2010-07-01 14:41:37 EDT
Created attachment 428526 [details]
git patch file (9.0)

12241 Triaged Unassigned Bug Minor Fix Required
delete_acl_from_file(char *, char *…) ds/lib/libaccess/acltools.cpp
   
12242 UNINIT Triaged Unassigned Bug Minor Fix Required
get_acl_from_file(char *, char *, ACLListHandle **…) ds/lib/libaccess/acltools.cpp
   
12243 UNINIT Triaged Unassigned Bug Minor Fix Required
ACL_FileGetNameList() ds/lib/libaccess/acltools.cpp

12244 UNINIT Triaged Unassigned Bug Minor Fix Required
ACL_FileGetNameList() ds/lib/libaccess/acltools.cpp

12245 UNINIT Triaged Unassigned Bug Minor Fix Required
rename_acl_in_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp

12246 UNINIT Triaged Unassigned Bug Minor Fix Required
append_acl_to_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp

12247 UNINIT Triaged Unassigned Bug Minor Fix Required
append_acl_to_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp

Comment:
ACL_FileRenameAcl, ACL_FileDeleteAcl, ACL_FileGetAcl, ACL_FileSetAcl, ACL_FileMergeAcl, ACL_FileMergeFile and their helper functions are not used.  These functions and their helper functions plus libaccess test programs under the directory ./utest are eliminated.
Comment 19 Noriko Hosoi 2010-07-01 14:51:45 EDT
Note: the following coverity items are covered by the patch 
Created an attachment (id=428526) [details]
in Comment 18 (https://bugzilla.redhat.com/show_bug.cgi?id=609255#c18). 

12292 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
delete_acl_from_file(char *, char *…) ds/lib/libaccess/acltools.cpp

12294 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
get_acl_from_file(char *, char *, ACLListHandle **…)
ds/lib/libaccess/acltools.cpp

12296 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
ACL_FileGetNameList() ds/lib/libaccess/acltools.cpp

12298 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
rename_acl_in_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp    

12300 USE_AFTER_FREE Triaged Unassigned Bug Minor Fix Required
append_acl_to_file(char *, char *, char *…) ds/lib/libaccess/acltools.cpp
Comment 20 Rich Megginson 2010-07-01 16:08:11 EDT
Comment on attachment 427830 [details]
git patch file (9.0)

mods must be freed before returning.
Comment 21 Rich Megginson 2010-07-01 16:13:18 EDT
Comment on attachment 428142 [details]
git patch file (9.0)

hmm - are you sure this is ok to do?  this means 'a' can retain its previous value on the next loop iteration if there is a continue or some other condition that causes another loop iteration without freeing 'a'
Comment 22 Rich Megginson 2010-07-01 16:14:21 EDT
Comment on attachment 428526 [details]
git patch file (9.0)

I love getting rid of useless code . . .
Comment 23 Noriko Hosoi 2010-07-01 17:11:39 EDT
Created attachment 428563 [details]
revised patch (replacing attachment 427830 [details])

(In reply to comment #20)
> (From update of attachment 427830 [details])
> mods must be freed before returning.    

Thanks, Rich!  Attached is a revised patch.
Comment 24 Noriko Hosoi 2010-07-01 17:26:00 EDT
(In reply to comment #21)
> (From update of attachment 428142 [details])
> hmm - are you sure this is ok to do?  this means 'a' can retain its previous
> value on the next loop iteration if there is a continue or some other condition
> that causes another loop iteration without freeing 'a'    

Well, I tested just assigning NULL to 'a' at the original position in the loop, then I got this error many times:
str2entry_fast: Error. Non-contiguous attribute values for objectclass

Looking at the code around the error, NULL 'a' is used to determine the attribute values are non-contiguous.  If 'a' is NULL, the type is treated at the first time in the loop.
 382             if(a==NULL)
 383             {
 384                 switch(attr_state)
 385                 {
 386                 case ATTRIBUTE_PRESENT:
 387                     if(attrlist_find_or_create_locking_optional(&e->e_attrs, type, &a, PR_FALSE)==0 /* Found */)
 388                     {
 389                         LDAPDebug (LDAP_DEBUG_ANY, "str2entry_fast: Error. Non-contiguous attribute values for %s\n", type, 0, 0);
 390                         PR_ASSERT(0);
 391                         if (freetype) slapi_ch_free_string(&type);
 392                         continue;
 393                     }
 394                     break;

Please note that str2entry_fast sets NULL to 'a', at the first time or type has been changed since the previous loop.
 264         if((ptype==NULL)||(strcasecmp(type,ptype) != 0))
 265         {
 266             slapi_ch_free_string(&ptype);
 267             ptype=slapi_ch_strdup(type);
 268             nvals = 0;
 269             maxvals = 0;
 270             del_nvals = 0;
 271             del_maxvals = 0;
 272             a = NULL;
 273         }
Comment 25 Noriko Hosoi 2010-07-07 20:28:35 EDT
Reviewed by Rich (Thanks!!)

Pushed to master.

$ git merge coverity
Updating 85eb921..e9a26dc
Fast-forward
 include/libaccess/aclproto.h                       |    7 -
 ldap/servers/plugins/replication/cl5_api.c         |   22 +-
 ldap/servers/plugins/replication/repl5_plugins.c   |    2 +-
 ldap/servers/plugins/replication/repl5_protocol.c  |    2 +-
 ldap/servers/plugins/replication/repl5_total.c     |    2 +-
 ldap/servers/plugins/replication/repl_extop.c      |    4 +-
 ldap/servers/plugins/replication/windows_private.c |    4 +-
 ldap/servers/plugins/uiduniq/7bit.c                |    6 +-
 ldap/servers/plugins/uiduniq/uid.c                 |    6 +-
 ldap/servers/slapd/back-ldbm/dblayer.c             |   12 +-
 ldap/servers/slapd/back-ldbm/vlv.c                 |    2 +-
 ldap/servers/slapd/entry.c                         |    2 +-
 lib/ldaputil/dbconf.c                              |    1 +
 lib/libaccess/acltools.cpp                         |  768 +-------------------
 lib/libaccess/oneeval.cpp                          |    8 +-
 lib/libaccess/utest/.purify                        |   19 -
 lib/libaccess/utest/Makefile                       |  147 ----
 lib/libaccess/utest/acl.dat                        |   44 --
 lib/libaccess/utest/aclfile0                       |   87 ---
 lib/libaccess/utest/aclfile1                       |   43 --
 lib/libaccess/utest/aclfile10                      |   45 --
 lib/libaccess/utest/aclfile11                      |   43 --
 lib/libaccess/utest/aclfile12                      |   43 --
 lib/libaccess/utest/aclfile13                      |   43 --
 lib/libaccess/utest/aclfile14                      |   43 --
 lib/libaccess/utest/aclfile15                      |   43 --
 lib/libaccess/utest/aclfile16                      |   43 --
 lib/libaccess/utest/aclfile17                      |   43 --
 lib/libaccess/utest/aclfile18                      |   51 --
 lib/libaccess/utest/aclfile19                      |   46 --
 lib/libaccess/utest/aclfile2                       |   43 --
 lib/libaccess/utest/aclfile3                       |   43 --
 lib/libaccess/utest/aclfile4                       |   43 --
 lib/libaccess/utest/aclfile5                       |   43 --
 lib/libaccess/utest/aclfile6                       |   55 --
 lib/libaccess/utest/aclfile7                       |   43 --
 lib/libaccess/utest/aclfile8                       |   43 --
 lib/libaccess/utest/aclfile9                       |   43 --
 lib/libaccess/utest/aclgrp0                        |   42 -
 lib/libaccess/utest/aclgrp1                        |   42 -
 lib/libaccess/utest/aclgrp2                        |   42 -
 lib/libaccess/utest/aclgrp3                        |   42 -
 lib/libaccess/utest/aclgrp4                        |   42 -
 lib/libaccess/utest/acltest.cpp                    |  794 --------------------
 lib/libaccess/utest/onetest.cpp                    |   77 --
 lib/libaccess/utest/shexp.cpp                      |  331 --------
 lib/libaccess/utest/shexp.h                        |  168 -----
 lib/libaccess/utest/test.ref                       |  217 ------
 lib/libaccess/utest/testmain.cpp                   |   89 ---
 lib/libaccess/utest/twotest.cpp                    |   87 ---
 lib/libaccess/utest/ustubs.cpp                     |  331 --------
 51 files changed, 51 insertions(+), 4240 deletions(-)
 delete mode 100644 lib/libaccess/utest/.purify
 delete mode 100644 lib/libaccess/utest/Makefile
 delete mode 100644 lib/libaccess/utest/acl.dat
 delete mode 100644 lib/libaccess/utest/aclfile0
 delete mode 100644 lib/libaccess/utest/aclfile1
 delete mode 100644 lib/libaccess/utest/aclfile10
 delete mode 100644 lib/libaccess/utest/aclfile11
 delete mode 100644 lib/libaccess/utest/aclfile12
 delete mode 100644 lib/libaccess/utest/aclfile13
 delete mode 100644 lib/libaccess/utest/aclfile14
 delete mode 100644 lib/libaccess/utest/aclfile15
 delete mode 100644 lib/libaccess/utest/aclfile16
 delete mode 100644 lib/libaccess/utest/aclfile17
 delete mode 100644 lib/libaccess/utest/aclfile18
 delete mode 100644 lib/libaccess/utest/aclfile19
 delete mode 100644 lib/libaccess/utest/aclfile2
 delete mode 100644 lib/libaccess/utest/aclfile3
 delete mode 100644 lib/libaccess/utest/aclfile4
 delete mode 100644 lib/libaccess/utest/aclfile5
 delete mode 100644 lib/libaccess/utest/aclfile6
 delete mode 100644 lib/libaccess/utest/aclfile7
 delete mode 100644 lib/libaccess/utest/aclfile8
 delete mode 100644 lib/libaccess/utest/aclfile9
 delete mode 100644 lib/libaccess/utest/aclgrp0
 delete mode 100644 lib/libaccess/utest/aclgrp1
 delete mode 100644 lib/libaccess/utest/aclgrp2
 delete mode 100644 lib/libaccess/utest/aclgrp3
 delete mode 100644 lib/libaccess/utest/aclgrp4
 delete mode 100644 lib/libaccess/utest/acltest.cpp
 delete mode 100644 lib/libaccess/utest/onetest.cpp
 delete mode 100644 lib/libaccess/utest/shexp.cpp
 delete mode 100644 lib/libaccess/utest/shexp.h
 delete mode 100644 lib/libaccess/utest/test.ref
 delete mode 100644 lib/libaccess/utest/testmain.cpp
 delete mode 100644 lib/libaccess/utest/twotest.cpp
 delete mode 100644 lib/libaccess/utest/ustubs.cpp

$ git push
Counting objects: 137, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (109/109), done.
Writing objects: 100% (109/109), 14.14 KiB, done.
Total 109 (delta 80), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
   85eb921..e9a26dc  master -> master

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