Experimental Physics and
Industrial Control System

"Johnson, Andrew N. via Core-talk" <core-talk at aps.anl.gov> · Wed, 12 Feb 2020 11:37:36 +0000

Hi Karl,

> On Feb 12, 2020, at 2:57 AM, Karl Vestin via Core-talk <core-talk at aps.anl.gov> wrote:
> 
> Steps to improve signal to noise ratio:
> 1) Flawfinder is reporting all use of standard string functions (e.g. strncpy) as Security issues of Info severity. We probably want to be able to use standard string functions. Removing the rules for this reduced the number of security issues by 331.
> Total after: 737

If those are general “string functions are always bad” rules then they’re not very useful for us, we probably aren’t going to (be able to) replace them and we’ve been using them for so long that most of our uses should be safe.

> 2) Markdown syntax checker may find issues in the markdown documentation, but this is probably not the issues we are looking for. Removing the patterns for markdown removes 111 issues.
> Total: 626

Sometime we might want to look at them, but in general I’ve been using GFM (GitHub Flavored Markdown) which I believe has slightly different rules. Don’t know if you can configure the checks for that; at some point we might want to be able to fix those, but Markdown is relatively new in EPICS Base.

> 3) cppchecker generates a Security issue of Info level whenever it detects that the scope of any variable could be smaller than it actually is. This is not expected to generate any functional impact. Removing this rule removes 215 issues.
> Total: 411

Agreed.

> 4) Flawfinder generates a Security issue of Info severity every time you copy something into a buffer in a loop, even when the buffer is a single character (such as when reading an input stream). Removing these patterns removes 56 issues.
> Total: 355

Ack, seems pointless.

> 5) cppcheck is not always able to resolve macros and environment variables. When it cannot run another check due to not being able to resolve a macro or environment variable it generates a Code Style issue of Info severity. Removing this rule clears another 42 issues.
> Total: 313

Ack.

> 6) The test code generates a fair number of issues. This is to be expected since test code generally does things that normally would seems nonsensical. Ignoring the test code folders removed an additional 54 issues.
> Total: 259

Initially this makes sense. We might want to revisit it in the future.

> In the analysis 24 issues are identified by the tool as "Errors" (highest level). These fall into a few categories:
>  
> - Memory leaks. This is typically due to missing free statements before returning in error cases. Likely to have limited or no impact, but very easy to fix.

Actually this is very helpful for long-running IOCs that don’t have much memory.

> - Possible null pointer dereferencing. In a handful of instances pointers are dereferenced without NULL check. The pointer should not be NULL, so practical impact is probably low. But correcting is again very easy.

If the routine is static (so only visible to other routines inside the same source file) I would prefer to not add unnecessary NULL tests, but for routines that are meant to be a public API NULL tests probably should be added. IMHO, others may have a different opinion…

> - Uninitialized variables or struct fields. Occurs in a few places. Should probably be fixed.

Agreed.

> - Potential memory leak in case of realloc failing. If realloc fails the original memory still needs to be freed. Since realloc rarely fails this probably has very low practical impact.

I see these in yacc/reader.c which is (old) code imported from outside and is only run at build-time. I would ignore these here, maybe ignore that directory (and the adjacent flex directory if it shows similar warnings).

> - cppchecker generates an Error severity issue whenever a #include statement does not include a header file. The modules/libcom/vxWorks/boost/config.hpp file declares a handful of includes using macros. I have not dug further into the issues, but I think they can safely be ignored. But since the issue is "Error" level I would like some feedback on this before I just ignore the issue.

The libcom/vxWorks/boost headers are not intended to be a complete installation of Boost, so there are missing files there which we never use. Ignore.

> - The startup/unix.sh does not include a shebang, hence the tools cannot determine if it works. I think this is "working as desiged" and should be ignored. But since the issue is "Error" level I would like some feedback on this before I just ignore the issue.

Agree, ignore. This file is supposed to be sourced to set up ones environment, not run.

> - A single instance of incorrect number of parameters for printf format string. Should just be fixed, looks like a simple mistake.

Ack.

> Some areas where I would like input:
>  
> 1)      Is this time well spent?

Yes.

> 2)      Do you agree with my analysis of the Error level issues above?

Yes.

> 3)      If this is a tool we want to pursue, how to manage the configuration? Codacy does support configuration files that can be stored in the root of the source repository, but I have yet to dig into the formatting of such files. Excluding files and directories can certainly be managed, but I am less certain about the configuration of the analysis itself (such as ignoring specific rules).

Codacy’s configuration settings on the website are presumably by project/GitHub repo. We use multiple branches in a single project, but I think it’s reasonable to keep the same settings for all branches. We already have a significantly different number of tests between branches, with 7 being better than 3.16 than 3.15 than 3.14 so we’re used to this kind of difference. Thus I’m fine with the website configuration settings until we get a bit more experience.

> 4)      Finally, if we do decide to pursue this tool how do we visualize and use the results?

Codacy results are already being displayed by GitHub on Pull Requests[1], thanks to Ralph. Our top-level README file isn’t currently a .md file but we could make it one and add badges there. We can also add links to appropriate places in the EPICS websites. What other outputs are possible? I don’t know Codacy’s capabilities.

[1] https://github.com/epics-base/epics-base/pulls

Thanks,

- Andrew

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System