Bug 691422 - fix coverity control flow issues
Summary: fix coverity control flow issues
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Directory Server
Version: 1.2.8
Hardware: Unspecified
OS: Unspecified
medium
unspecified
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-28 14:27 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:08:11 UTC


Attachments (Terms of Use)
0001-Bug-691422-sdt_destroy-fix-coverity-control-flow-iss.patch (1.08 KB, patch)
2011-03-28 20:50 UTC, Rich Megginson
nkinder: review+
Details | Diff
0002-Bug-691422-ldbm_back_upgradedb-fix-coverity-control-.patch (1.14 KB, patch)
2011-03-28 20:51 UTC, Rich Megginson
nkinder: review+
Details | Diff
0003-Bug-691422-csnplFree-fix-coverity-control-flow-issue.patch (1.09 KB, patch)
2011-03-28 20:51 UTC, Rich Megginson
nkinder: review+
Details | Diff
0004-Bug-691422-SetUnicodeStringFromUTF_8-fix-coverity-co.patch (1.25 KB, patch)
2011-03-28 20:53 UTC, Rich Megginson
nkinder: review+
Details | Diff
0005-Bug-691422-cl5DeleteRUV-fix-coverity-control-flow-is.patch (1.35 KB, patch)
2011-03-28 20:53 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0006-Bug-691422-acl_read_access_allowed_on_entry-fix-cove.patch (2.79 KB, patch)
2011-03-28 20:54 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0007-Bug-691422-search_internal_callback_pb-fix-coverity-.patch (1.44 KB, patch)
2011-03-28 20:54 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0008-Bug-691422-cl5WriteRUV-fix-coverity-control-flow-iss.patch (1.13 KB, patch)
2011-03-28 20:55 UTC, Rich Megginson
nhosoi: review+
Details | Diff
0009-Bug-691422-windows_replay_update-fix-coverity-contro.patch (1.53 KB, patch)
2011-03-28 20:55 UTC, Rich Megginson
nhosoi: review+
Details | Diff

Description Rich Megginson 2011-03-28 14:27:27 UTC
fix coverity control flow issues

Comment 1 Rich Megginson 2011-03-28 14:28:59 UTC
10713: STRAY_SEMICOLON
sdt_destroy
"if" statement "if (sdt->dns /* != 0 */);" with no "then" or "else" is suspicious. Is the ';' after "if (sdt->dns)" extraneous?
  99    if (sdt->dns);
 100        free(sdt->dns);
 101    if (sdt->uids);
 102        free(sdt->uids);
 103    free(sdt);

Comment 2 Rich Megginson 2011-03-28 14:30:16 UTC
"if" statement "if (rval /* != 0 */);" with no "then" or "else" is suspicious. Is the ';' after "if (rval)" extraneous?
2763            if (rval);
2764            {
2765                upgrade_rval += rval;
2766                slapi_log_error(SLAPI_LOG_FATAL, "upgrade DB",
2767                    "Can't clean up indices in %s\n", inst->inst_dir_name);
2768                continue; /* Need to make all backups; continue */
2769            }

Comment 3 Rich Megginson 2011-03-28 14:33:02 UTC
ldbm_back_upgradedb
(In reply to comment #2)
> "if" statement "if (rval /* != 0 */);" with no "then" or "else" is suspicious.
> Is the ';' after "if (rval)" extraneous?
> 2763            if (rval);
> 2764            {
> 2765                upgrade_rval += rval;
> 2766                slapi_log_error(SLAPI_LOG_FATAL, "upgrade DB",
> 2767                    "Can't clean up indices in %s\n", inst->inst_dir_name);
> 2768                continue; /* Need to make all backups; continue */
> 2769            }

Comment 4 Rich Megginson 2011-03-28 14:33:22 UTC
"if" statement "if ((*csnpl)->csnLock /* != 0 */);" with no "then" or "else" is suspicious. Is the ';' after "if ((*csnpl)->csnLock)" extraneous?
 125        if ((*csnpl)->csnLock);
 126                PR_DestroyRWLock ((*csnpl)->csnLock);

Comment 5 Rich Megginson 2011-03-28 14:33:56 UTC
csnplFree
(In reply to comment #4)
> "if" statement "if ((*csnpl)->csnLock /* != 0 */);" with no "then" or "else" is
> suspicious. Is the ';' after "if ((*csnpl)->csnLock)" extraneous?
>  125        if ((*csnpl)->csnLock);
>  126                PR_DestroyRWLock ((*csnpl)->csnLock);

Comment 6 Rich Megginson 2011-03-28 14:35:31 UTC
SetUnicodeStringFromUTF_8
This less-than-zero comparison of an unsigned value is never true. "n < 0UL".
 281    if (n < 0) { /* bogus */
 282        return U_INVALID_FORMAT_ERROR; /* don't know what else to use here */
 283    }

Comment 7 Rich Megginson 2011-03-28 15:03:56 UTC
cl5DeleteRUV
Noticing condition "file_obj".
After this line, the value of "file_obj" is equal to 0.
6378    while (file_obj) {
...
On this path, the condition "file_obj" cannot be true.
6404    if (file_obj) {
Execution cannot reach this statement "object_release(file_obj);".
6405        object_release (file_obj);
6406    }

Comment 8 Rich Megginson 2011-03-28 15:09:41 UTC
acl_read_access_allowed_on_entry
the attr_index and code that uses it was left over from the DETERMINE_ACCESS_BASED_ON_REQUESTED_ATTRIBUTES code which was commented out long ago - we should just remove all code that uses attr_index - it will always be -1 so the code that refers to it is dead code

Comment 9 Rich Megginson 2011-03-28 15:11:30 UTC
search_internal_callback_pb
Execution cannot reach this expression "NULL" inside statement "filter = slapi_str2filter((...".
On this path, the condition "ifstr" cannot be false.
 722    filter = slapi_str2filter(ifstr ? (fstr = slapi_ch_strdup(ifstr)) : NULL);

just get rid of the ifstr ? - it is ok to pass a NULL to slapi_ch_strdup anyway.

Comment 10 Rich Megginson 2011-03-28 15:12:41 UTC
cl5WriteRUV
Noticing condition "file_obj".
After this line, the value of "file_obj" is equal to 0.
6296    while (file_obj) {
...
On this path, the condition "file_obj" cannot be true.
6305    if (file_obj) {
Execution cannot reach this statement "object_release(file_obj);".
6306        object_release (file_obj);

Comment 11 Rich Megginson 2011-03-28 15:14:19 UTC
windows_replay_update
Assigning: "is_ours" = "0".
After this line, the value of "is_ours" is equal to 0. 
1373        int is_ours = 0;
...
Execution cannot reach this expression ""ours"" inside statement "slapi_log_error(12, repl_pl...".
On this path, the condition "is_ours" cannot be true.
1423                                slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name,
1424                                        "%s: windows_replay_update: Looking at %s operation local dn=\"%s\" (%s)\n",
1425                                        agmt_get_long_name(prp->agmt),
1426                                        op2string(op->operation_type), op->target_address.dn, is_ours ? "ours" : "not ours");

Comment 12 Rich Megginson 2011-03-28 20:50:02 UTC
Created attachment 488254 [details]
0001-Bug-691422-sdt_destroy-fix-coverity-control-flow-iss.patch

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

Comment 13 Rich Megginson 2011-03-28 20:51:17 UTC
Created attachment 488255 [details]
0002-Bug-691422-ldbm_back_upgradedb-fix-coverity-control-.patch

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

Comment 14 Rich Megginson 2011-03-28 20:51:46 UTC
Created attachment 488256 [details]
0003-Bug-691422-csnplFree-fix-coverity-control-flow-issue.patch

https://bugzilla.redhat.com/show_bug.cgi?id=691422#c5

Comment 15 Rich Megginson 2011-03-28 20:53:10 UTC
Created attachment 488258 [details]
0004-Bug-691422-SetUnicodeStringFromUTF_8-fix-coverity-co.patch

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

Comment 16 Rich Megginson 2011-03-28 20:53:40 UTC
Created attachment 488259 [details]
0005-Bug-691422-cl5DeleteRUV-fix-coverity-control-flow-is.patch

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

Comment 17 Rich Megginson 2011-03-28 20:54:08 UTC
Created attachment 488260 [details]
0006-Bug-691422-acl_read_access_allowed_on_entry-fix-cove.patch

https://bugzilla.redhat.com/show_bug.cgi?id=691422#c8

Comment 18 Rich Megginson 2011-03-28 20:54:45 UTC
Created attachment 488261 [details]
0007-Bug-691422-search_internal_callback_pb-fix-coverity-.patch

https://bugzilla.redhat.com/show_bug.cgi?id=691422#c9

Comment 19 Rich Megginson 2011-03-28 20:55:24 UTC
Created attachment 488263 [details]
0008-Bug-691422-cl5WriteRUV-fix-coverity-control-flow-iss.patch

https://bugzilla.redhat.com/show_bug.cgi?id=691422#c10

Comment 20 Rich Megginson 2011-03-28 20:55:53 UTC
Created attachment 488264 [details]
0009-Bug-691422-windows_replay_update-fix-coverity-contro.patch

https://bugzilla.redhat.com/show_bug.cgi?id=691422#c11

Comment 21 Rich Megginson 2011-03-28 21:23:26 UTC
To ssh://git.fedorahosted.org/git/389/ds.git
   c21b037..67e2651  master -> master
commit 67e2651847a7a1afbd307462106a23804ed99f62
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 14:19:24 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: is_ours will always be true here - so just remove the ?: statement and hard code "ours"
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 09f09816352f18e799abe5e25916f6c375b64bbf
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 14:16:32 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: file_obj will always be NULL here, so just remove the code
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 3cb50e12fec36e32c738f08b4d9f0baef341f916
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 14:14:50 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: just get rid of the ifstr ?: - it is ok to pass a NULL to s
lapi_ch_strdup anyway.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 90919bb41f89cfc376d170c4b0b6c790b7ba96d3
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 14:12:13 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: ifdef out all code related to DETERMINE_ACCESS_BASED_ON_REQ
UESTED_ATTRIBUTES
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit e936bd91f910825918b6b2951add7bc37c1a814b
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 14:08:26 2011 -0600
    Reviewed by: nhosoi (Thanks!)
    Branch: master
    Fix Description: move the if (file_obj) clause after the bail: label - I thi
nk this
    was the intention of the code - if the while loop was bailed, file_obj may b
e
    acquired, so it would have to be released
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit be6ee97e7ee238fc03aea74ab3e5e37537fc6e13
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 13:57:53 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: n is size_t, which is unsigned.  In this case, n cannot be 
less
    than zero anyway, so comparison with 0 will suffice.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 1f23a16771a75b958850220c166af373e32457d8
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 12:39:05 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: remove spurious semicolon
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 5ab6c376a9339aef68c492f8bcf657b28a662a22
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 12:33:58 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: remove spurious semicolon
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no
commit 45189c533b46292bcea0adcfba457d292ef23a3d
Author: Rich Megginson <rmeggins@redhat.com>
Date:   Mon Mar 28 12:32:28 2011 -0600
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: remove spurious semicolons
    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.