#48048 Fix coverity issues - 2015/2/24, 2015/3/1
Closed: wontfix None Opened 9 years ago by nhosoi.

No Description Provided


Replying to [comment:3 rmeggins]:

in https://fedorahosted.org/389/attachment/ticket/48048/0006-Ticket-48048-Fix-coverity-issues-2015-2-24.patch and https://fedorahosted.org/389/attachment/ticket/48048/0032-Ticket-48048-Fix-coverity-issues-2015-2-24.patch - can you just use
{{{
PRUint64 o_connid = 0xffffffffffffffff; / no op /
}}}
and avoid having to cast anything?

Yeah, I'd also like to avoid casting. The original code is the old line 305. o_connid has type PRUint64, which is casted to (long long unsigned int) for "conn=%" NSPRIu64. Those casts are now removed as in the new line 310....
{{{
302 307 slapi_log_error (loglevel, plugin_name,
303 308 "conn=%" NSPRIu64 " op=%d (main): Deny %s on entry(%s)"
304 ": readonly backend\n",
305 (long long unsigned int)op->o_connid, op->o_opid,
309 ": readonly backend\n",
310 o_connid, o_opid,
}}}

in https://fedorahosted.org/389/attachment/ticket/48048/0018-Ticket-48048-Fix-coverity-issues-2015-2-24.patch
Is rc initialized or otherwise set?
rc is initialized with -1.
1185 int rc = -1;

Right. You got rid of all of the casting except this one:
{{{

265         if (op) { 
266                 o_connid = (long long unsigned int)op->o_connid; 
267                 o_opid = op->o_opid; 
268         }

}}}
Can you declare PRUint64 o_connid instead of long long unsigned int o_connid? Then you shouldn't need the cast here because
{{{
typedef struct op {
...
PRUint64 o_connid; / id of conn initiating this op; for logging only /
}}}

Replying to [comment:5 rmeggins]:

Right. You got rid of all of the casting except this one:

Ah, I see. You want to avoid all of them... :) Let me try the change...

Replying to [comment:6 nhosoi]:

Replying to [comment:5 rmeggins]:

Right. You got rid of all of the casting except this one:

Ah, I see. You want to avoid all of them... :) Let me try the change...

Well, now, we have to face the original compiler warning problem. (we put lots of cast in slapi_log_error to work around this...)

ldap/servers/plugins/acl/acl.c:312:5: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 4 has type 'PRUint64' [-Wformat=]

slapi-plugin.h:#define NSPRIu64 "llu"

260 PRUint64 o_connid = 0xffffffffffffffff; / no op /
...
266 o_connid = op->o_connid;
...
307 slapi_log_error (loglevel, plugin_name,
308 "conn=%" NSPRIu64 " op=%d (main): Deny %s on entry(%s)"
309 ": readonly backend\n",
310 o_connid, o_opid,
311 acl_access2str(access),
312 n_edn);

Would you mind introducing this "#ifdef CPU_x86_64"? This cleans up the above warning.
{{{
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index bc68157..c559852 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -89,8 +89,13 @@ NSPR_API(PRUint32) PR_fprintf(struct PRFileDesc fd, const char fmt, ...)
/ NSPR uses the print macros a bit differently than ANSI C. We
* need to use ll for a 64-bit integer, even when a long is 64-bit.
/
+#ifdef CPU_x86_64
+#define NSPRIu64 "lu"
+#define NSPRI64 "l"
+#else
#define NSPRIu64 "llu"
#define NSPRI64 "ll"
+#endif
}}}

Ok, I see what we want - If PRUint64 is defined as unsigned log, use %lu, otherwise, use %llu. We should use the same macro that prtypes.h uses:
{{{

ifdef HAVE_LONG_LONG

/ Keep this in sync with prlong.h. /

if PR_BYTES_PER_LONG == 8 && !defined(PR_ALTERNATE_INT64_TYPEDEF)

typedef long PRInt64;
typedef unsigned long PRUint64;
}}}

So we could do something like this:
{{{

if defined(HAVE_LONG_LONG) && PR_BYTES_PER_LONG == 8 && !defined(PR_ALTERNATE_INT64_TYPEDEF)

define NSPRIu64 "lu"

define NSPRI64 "l"

else / just assume long long is used /

define NSPRIu64 "llu"

define NSPRI64 "ll"

endif

}}}

Yep, it works nicely. I'm putting the change to one of the patches (​https://fedorahosted.org/389/attachment/ticket/48048/0006-Ticket-48048-Fix-coverity-issues-2015-2-24.patch) and modifying the rest by getting rid of the casts (I think there are more outside of the patches I proposed...)

Thanks a lot, Rich!!

git patch file (master) -- get rid of (long long unsigned int) cast
0006-Ticket-48048-Fix-coverity-issues-2015-2-24.2.patch

git patch file (master) -- get rid of (long long unsigned int) cast
0032-Ticket-48048-Fix-coverity-issues-2015-2-24.2.patch

git patch file (master) -- get rid of (long long unsigned int) cast
0041-Ticket-48048-Fix-coverity-issues-2015-2-24.patch

Reviewed and advised by Rich (Thank you!!)

Pushed to master:
1aeaf34..1f59d61 master -> master
commit 1f59d61
commit cd0e901
commit 52d2f9a
commit c1a8ba4
commit 9ae878c
commit c1fa27d
commit 3e3ed48
commit c703a2b
commit f0d49d2
commit b9723db
commit 26f1ba5
commit 500e8be
commit 13184ae
commit bbf6520
commit f3b463f
commit deaac70
commit acc0a29
commit c88c727
commit 2263bd9
commit b6d259d
commit 0cbd5bd
commit 4d60b68
commit 18b44d1
commit 76b34e1
commit a7eb4e0
commit 1b0d58a
commit 588ea1f
commit 7348992
commit f91b582
commit 6cb6ef1
commit bb5b5be
commit 6be713c
commit c097f15
commit d1574aa
commit 3bf2d59
commit fc0036e
commit 7039bca
commit 94d4b10
commit a10a1de
commit 5ce33c4

Pushed to 389-ds-base-1.3.3:
79a8898..27c382f 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 27c382f
commit cc5aba3
commit c1190b4
commit 6702462
commit 20ef9c9
commit 8c84bdf
commit 13da51e
commit 9542aa4
commit b224420
commit 0b47821
commit 1acdd9b
commit 29260b7
commit c080567
commit 2e7ed6a
commit 26d795e
commit 9c33372
commit 3f7e0d4
commit 2b6c686
commit 2828f8d
commit df83022
commit 9213253
commit 8cf9000
commit a50a176
commit b9cd9c9
commit 4014a80
commit 7c408d3
commit 5f34a00
commit 3e5066f
commit f8c78d8
commit 350dd7e
commit 3763476
commit 4122156
commit fcfceb2
commit 62c01b7
commit 17eb3cb
commit e52e756
commit 0dec9f1
commit 6974f19
commit 2a571d5
commit 97cf727

One more for PRInt64; getting rid of casting to (long long int)
Master:
1f59d61..8f1eef1 master -> master
commit 8f1eef1

389-ds-base-1.3.3:
27c382f..a875da1 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit a875da1

Thanks for reviewing the patch, Rich!!

Pushed to master:
3701875..f6eeaf9 master -> master
commit f6eeaf9

Pushed to 389-ds-base-1.3.3:
d6f7b97..393a939 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 393a939

Metadata Update from @nhosoi:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.3.3.9

7 years ago

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/1379

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata