*: make coverity scan ignore random() calls #6251
Conversation
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/272e9d03c8a99fae2b68edc5318cba77/raw/ebcfaff2d9e341b145bb123319b1983c653fc511/cr_6251_1587132393.diff | git apply
diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c
index 4c84aed32..fc2653884 100644
--- a/ospfd/ospf_nsm.c
+++ b/ospfd/ospf_nsm.c
@@ -723,7 +723,7 @@ static void nsm_change_state(struct ospf_neighbor *nbr, int state)
/* Start DD exchange protocol */
if (state == NSM_ExStart) {
if (nbr->dd_seqnum == 0)
- /* coverity[dont_call] */
+ /* coverity[dont_call] */
nbr->dd_seqnum = (uint32_t)random();
else
nbr->dd_seqnum++;
If you are a new contributor to FRR, please see our contributing guidelines.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11929/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
There was a problem hiding this comment.
Thanks, I hate it /s
This is fine but I think we could reduce littering pragma comments by making a wrapper? like frr_unsafe_random or frr_jitter, and then just keep the coverity pragma in that one spot. Plus it becomes very obvious what the random() is used for then.
Replace all `random()` calls with a function called `frr_weak_random()` and make it clear that it is only supposed to be used for weak random applications. Use the annotation described by the Coverity Scan documentation to ignore `random()` call warnings. Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/95b78c3c504bb56468f386e5aece89a6/raw/20abea5366c0d355936ae90a29c5b567a9fbbad3/cr_6251_1587168229.diff | git apply
diff --git a/watchfrr/watchfrr.c b/watchfrr/watchfrr.c
index 2db612adc..0929cb34a 100644
--- a/watchfrr/watchfrr.c
+++ b/watchfrr/watchfrr.c
@@ -44,7 +44,7 @@
#endif
/* Macros to help randomize timers. */
-#define JITTER(X) ((frr_weak_random() % ((X)+1))-((X)/2))
+#define JITTER(X) ((frr_weak_random() % ((X) + 1)) - ((X) / 2))
#define FUZZY(X) ((X)+JITTER((X)/20))
#define DEFAULT_PERIOD 5
If you are a new contributor to FRR, please see our contributing guidelines.
I like your suggestion. Branch rebased to use that. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11937/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
@qlyoung code updated, mind if I push in? |
Summary
Use the annotation described by the Coverity Scan documentation to ignore all
random()calls. Those are used mostly to calculate jitter and we don't need anything fancy.Code Change Details
watchfrrhas some some macro calls annotated (e.g.SET_WAKEUP_UNRESPONSIVE) because they contain a call torandom().lib/qobj.chas an example of shared code annotation:Extra Details
If you have a coverity scan account, then you can read the documentation here:
https://scan.coverity.com/models#c_checker_checkerconfig
Go to section
A.1.8. Suppressing false positives with code annotations:After a Code Coverity issue is found, we can tag it with a comment containing the event (not the defect which is usually an upper case word) so it will be ignored automatically.
Example:
/* coverity[var_deref_op] */The comment must appear right before the code line we want to ignore.
Be careful when annotating code with shared defects.
The code annotation will mark the code as
intentional, however it also supports "FALSE".Example:
/* coverity[var_deref_op : FALSE] */