CI: add scheduled Coverity scan

[chipitsine]

No description provided.

[paulusmack]

@RICCIARDI-Adrien @enaess any comments on this PR?

[Neustradamus]

@paulusmack, @chipitsine: "--form email=[email protected]".

[chipitsine]

email is required field.
what email do you prefer to specify ?

[RICCIARDI-Adrien]

@chipitsine Even with another email address, will it be possible for people not having access to the email account to get access to the Coverity reports ?

[chipitsine]

@chipitsine Even with another email address, will it be possible for people not having access to the email account to get access to the Coverity reports ?

sure.

[chipitsine]

@RICCIARDI-Adrien , even now, scheduled scan is not available yet, I ran scan manually.
if you have capasity/will to review findings, I can add you account

[RICCIARDI-Adrien]

I'm not fond of a solution that is not fully open source, so maybe we can add continue-on-error: true (see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error) to the job, so the whole CI is not blocked if something goes wrong with this specific job (for instance, the free license changes).

[chipitsine]

It is scheduled CI, it does not block "on push" builds

…

On Mon, Oct 2, 2023, 20:16 Adrien RICCIARDI ***@***.***> wrote: I'm not fond of a solution that is not fully open source, so maybe we can add continue-on-error: true (see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error) to the job, so the whole CI is not blocked if something goes wrong with this specific job (for instance, the free license changes). — Reply to this email directly, view it on GitHub <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ5KUB6FZ3U22XFGFBG2JDX5LZFBAVCNFSM6AAAAAA3I55T3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBTGQZTGNBUGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[RICCIARDI-Adrien]

Oh yes, you are correct, forget my comment !

[paulusmack]

So, what happens if Coverity decides that there is an error? Just an email to somewhere, or what?

[chipitsine]

coverity has nice web UI

it is already setup, I can add you to project (I need your email).

project members can browse issue in UI, also email is sent to them on new issues

[paulusmack]

project members can browse issue in UI, also email is sent to them on new issues

Please add me, [email protected].

How are the coverity scans done today? Do you do some manual step to trigger a scan, or something?

What are our options in the situations where coverity says something is an error but in fact it's correct? We have seen such situations in the past.

[chipitsine]

project members can browse issue in UI, also email is sent to them on new issues

Please add me, [email protected].

How are the coverity scans done today? Do you do some manual step to trigger a scan, or something?

What are our options in the situations where coverity says something is an error but in fact it's correct? We have seen such situations in the past.

there are few ways.

1. you can mark finding as "intentional" or "false positive"

2. alternatively, false positive may be marked in code comments https://community.synopsys.com/s/article/Suppressing-False-Positive-Intentional-defects

3. or via model https://community.synopsys.com/s/article/How-to-write-a-function-model-to-eliminate-a-false-positive-in-a-C-applilcation

[chipitsine]

project members can browse issue in UI, also email is sent to them on new issues

Please add me, [email protected].

How are the coverity scans done today? Do you do some manual step to trigger a scan, or something?

I've created CI task in my fork: https://github.com/chipitsine/ppp/tree/coverity
I merge master branch from time to time

[chipitsine]

project members can browse issue in UI, also email is sent to them on new issues

Please add me, [email protected].

I sent an invitation (maybe it reached spam folder)

[paulusmack]

The invitation did go to spam, but I found it.

Seems like coverity is complaining nearly every time we use warn() (or info(), fatal(), error(), etc.) because of the existence of implementations of those functions in pppd/plugins/pppoe/pppoe-discovery.c, which of course are not the implementations that get compiled into pppd. I guess it has no understanding of makefiles and just blindly assumes all source files are compiled into one big mess.

[chipitsine]

The invitation did go to spam, but I found it.

Seems like coverity is complaining nearly every time we use warn() (or info(), fatal(), error(), etc.) because of the existence of implementations of those functions in pppd/plugins/pppoe/pppoe-discovery.c, which of course are not the implementations that get compiled into pppd. I guess it has no understanding of makefiles and just blindly assumes all source files are compiled into one big mess.

it does not make any assumption.
it just follows the build process

[paulusmack]

it does not make any assumption. it just follows the build process

So why does it think that the warn() called from various places in ipcp.c could ever be the warn() defined in pppoe-discovery.c? Those two files are never compiled into the same executable. The calls in ipcp.c are to the warn() defined in utils.c. But when you look at the coverity errors, it is clearly linking the call in ipcp.c to the function in pppoe-discovery.c.

[chipitsine]

there's non zero probabilty that "ppp" configures itself to use plugins from pppoe folder.

I ran steps from CI, in folder "pppd" I see:

~/ppp/pppd$ grep -i pppoe Makefile | grep -i include
DEFAULT_INCLUDES = -I. -I$(top_builddir)/pppd/plugins/pppoe
~/ppp/pppd$ 

[paulusmack]

there's non zero probabilty that "ppp" configures itself to use plugins from pppoe folder.

Sure... and?

The point is that coverity is putting together a use of a function (in this case, warn()) in pppd with a definition which is not part of pppd or the pppoe plugin. To say it a different way, the code in pppoe-discovery.c (which contains an implementation of warn()) never appears in the address space of a pppd process. It is not part of pppd and it is not part of the pppoe plugin. What it is, is part of a separate standalone program which happens to use some of the same source code as the pppoe plugin (but note that the shared code does not include pppoe-discovery.c).

The result of coverity not understanding what gets linked with what is that it complains about things that are perfectly fine, making it quite hard to see what things do actually need attention.

I have no problem with coverity combining the code for pppd with the code for the pppoe plugin. But not all the source code in the pppd/plugins/pppoe directory goes into the pppoe plugin.

[paulusmack]

I just saw a new batch of scan results via email. They all seem to be predicated on the assumption that read() can return absolutely any integer, whereas by definition the return value is bounded by the 3rd argument, or is -1 indicating error. (And if the kernel is sufficiently broken that it is returning large positive values from read() then we have much bigger problems at hand than a few integer overflows.) The only one that isn't completely bogus points out a place where we don't check for error when reading /dev/urandom, which we should fix.

I remain unimpressed by the signal-to-noise ratio of these coverity scan results.

[paulusmack]

Oh, and there were a couple of reports that would be valid if tdb->hash_size could ever be >= 2^31. But if you look at the code, it's only ever going to be 131.

[chipitsine]

coverity is trying to develop their software. not every time is successful.
they deployed overflow checks in june 2024, those integer overflow checks now are just noise

[chipitsine]

I ran new scan to find out whether this commit cb59395 is noticed by coverity

in fact, it was

[chipitsine]