Check static analyzer log

[mcrha]

I happen to get to a static analyzers' log, which may contain something useful for you, I hope. It was crated with 0.6.75, which had only the extern_c patch applied on top of the official release.

I do not know the libpst internals, otherwise I'd help with the addressing of the issues. Note the static analyzers have a lot of false positives and they can be (over) pendantic too, thus it can be the log doesn't contain anything useful for you. Note few of the warnings are marked [important].

[pabs3]

If the extern C patch hasn't been fully merged upstream already, please send a pull request for that.

If you don't want to deal with the GitHub web interface for sending the PR, I can recommend the official GitHub CLI tool for doing this:

https://cli.github.com/

Thanks for the static analysis report.

Which tool generated this report?

Does the tool have any of its own checks or does it delegate all checks to the tools mentioned at the end of the report; clang, coverity, cppcheck, gcc, shellcheck, unicontrol.

I don't know what unicontrol is, can you give a link to it?

I don't have access to coverity, but I don't see anything in the report that looks like it is from coverity.

I don't know how to use the gcc analyser, does it have something similar to scan-build from the clang analyser?

The rest of them I am able to run locally so I should be able to reproduce the report against the latest code in the repository.

I expect there are definitely things worth fixing in the report, but as you say it does look like there are some false positives.

Most of the issues look possible to fix without any knowledge of the libpst internals, so feel free to submit pull requests for them.

I had started a while ago on the cppcheck complaints, take a look at the tiny cppcheck-fixes branch in my fork.

I'll leave this issue open as a reminder of the remaining work to be done and improve the code gradually over time.

-- bye, pabs

https://bonedaddy.net/pabs3/

[mcrha]

If the extern C patch hasn't been fully merged upstream already, please send a pull request for that.

An interesting thing about this patch is that none of the Fedora branches have it. They use 0.6.76, from which I guess the patch is applied to the libpst in some form.

Does the tool have any of its own checks or does it delegate all checks to the tools mentioned at the end of the report; clang, coverity, cppcheck, gcc, shellcheck, unicontrol.

It's all delegated to those listed tools. I do not know any details about any of those, this is just ran internally as part of a package checking. If you wish, I can re-run the tests manually with any patch (and with any version of the libpst), to see whether any proposed change would help.

[pabs3]

@mcrha I fixed a couple of issues in git main. Now I'm working on correcting all the printf conversion specifiers, especially the ones that should be using PRIx32 type size specs. If you could run the branch through your tool, that would help. There are still more fixes to make, but libpst.c should be done for the DEBUG_* macros.

https://github.com/pabs3/libpst/compare/correct-printf-specifiers

[mcrha]

I've a hard time to build the branch under Fedora: https://koji.fedoraproject.org/koji/taskinfo?taskID=86839765

checking whether to build the libpst python interface... yes
./configure: line 26383: syntax error near unexpected token `>'
./configure: line 26383: `    AX_PYTHON_DEVEL(>= '$PYTHON_VERSION')'

but even when I "fix that", there are no m4/ files (as they use to be in the official releases), thus it fails to find these Python and Boost AX macros, then it even fails on missing pyconfig.h.

[mcrha]

There is also missing content of the man directory (https://github.com/pst-format/libpst/tree/main/man), or it was not built here for some reason. Similarly the *.html files in the doc/ and doc/devel/ and the m4/* files. When I workaround these I can build it and the result is this scan-results.html

[pabs3]

Since the last release I removed all the embedded code copies of m4 files (since they are maintained by external projects not by libpst) from git and they are now expected to pulled in during autoreconf.

The release tarballs will of course contain them in line with autoconf tradition, but I encourage distros to use tarballs generated by GitHub or git archive instead, so that you know you can always rebuild the build system, because you do it on every build.

All the AX_* macros now come from the autoconf-archive package, so you will need that in your build dependencies if you don't have it yet.

This also means that the libpst customised AX_PYTHON_DEVEL is gone and so the operator is needed in its arguments, but strangely the version of AX_PYTHON_DEVEL you have requires no operator.

Which AX_PYTHON_DEVEL are you using in Fedora?

-- bye, pabs

https://bonedaddy.net/pabs3/

[pabs3]

The documentation building has always been a bit funky in libpst, the procedure listed in the README should work though. I am planning on making it work solely through autotools though.

Thanks for working through the issues and for the static analysis, I will take a look at the new results today.

-- bye, pabs

https://bonedaddy.net/pabs3/

[mcrha]

Which AX_PYTHON_DEVEL are you using in Fedora?

I had missing the right dependency, I believe. I just wanted to give you the Coverity Scan report quickly, which took me quite a long time at the end, but it was all about me not running the autoreconf properly, I guess. I resorted to several hacks to make it work "as before", without influencing the compilation itself.

[pabs3]

Sorry about that, I should have spent some more time on the build docs, so you didn't have to go through all those hacks :)

-- bye, pabs

https://bonedaddy.net/pabs3/

[pabs3]

@mcrha I've updated the branch again to fix the remaining printf argument warnings and all the mistakes I could see when auditing all the files for printf arguments.

The only issue I'm not sure about with the printf arguments is that some of the existing calls printf hex of signed integers. That seems like it is not supposed to be possible according to the printf docs. In the patch I changed them to decimal, but that changes the output format. Would it be better to cast signed integers to unsigned before passing to printf?

[mcrha]

You do not need to mention me, I'm still here :)

In the patch I changed them to decimal, but that changes the output format. Would it be better to cast signed integers to unsigned before passing to printf?

Hmm, changing output format can be problematic, if it's used by the callers. It depends on the actual use case, aka where it was done. I prefer to be on a safe side, thus I'd rather cast the value (an interesting thing would be to see what it did before the change, when the highest bit was set).

[pabs3]

Ah. Not everyone has notifications turned on for threads they participate in, so I usually @ mention people when needed.

The output changes would probably be in the debugging prints, so probably not used by any of the users of the library.

Not sure how feasible it is to check the high bit thing, but I will attempt to figure it out.

-- bye, pabs

https://bonedaddy.net/pabs3/

[mcrha]

Ah. Not everyone has notifications turned on for threads they participate in, so I usually @ mention people when needed.

I see. In that case do it as you are used to. It's not that critical here like on GitLab, where mentioning someone adds him/her a to-do, which I really do not like, because the to-do should be my list, managed by me, not by everybody.

The log at commit a3c033a27316d5cb5ba2e621338165076e4bcf4d is here: scan-results.html.txt weird it shows more things than before.

[pabs3]

Looks like all the PRINTF_ARGS issues got solved, but the solutions caused other issues in the form of compiler warnings about missing spaces between components of a string literal in C++11. Fixed.

I also went through the patch with a side-by-side coloured diff to ensure that all the changes make sense.

I read that interpreting a signed integer as an unsigned one is equivalent to a cast, so I elected to make that explicit by adding casts.I changed the type of some variables where a signed variable doesn't make sense.

I fixed some other things I missed too.

So I think the printf branch is now done. If you would like to review it too, that would be great.

[mcrha]

An updated scan-results.html is here. It's at commit 16ca2803b995b0a24e8c122dd723ab643ab9b779 .

I also built the package under Fedora Rawhide. Check the x86_64 and i686 build.log files, the 64bit claims 3 warnings, the 32bit 4 warnings (I searched for [-w in the file).

I did not do any real review of the changes, the patch is simply huge and I do not know the internals, variable types and so on. I'm sorry.

[pabs3]

@mcrha given csmock isn't available in Debian and requires RPM infrastructure anyway, what is the easiest way for me to be able to run it myself so I can iterate through the different issue classes without the extra latency of asking someone else to run it? Can I run it from a Fedora live image on a spare laptop for eg?

[pabs3]

I have rechecked, fixed and merged the patch fixing the printf format strings.

[mcrha]

I did not use any public Coverity Scan instance for testing, it's all internal, I'm sorry. Maybe you can add a project into https://scan.coverity.com/ just as libical or evolution-data-server, though I do not know what state these projects are in (the last check seems quite long time ago).

[pabs3]

Ah, I thought this was using open source RedHat tools that csmock runs, not the proprietary Coverity service.

Could you run it on git again to check if I got the printf fixes right?

-- bye, pabs

https://bonedaddy.net/pabs3/

[mcrha]

Sure, I can help. It's still kinda tricky to make it build from the git snapshot, because the man/ directory has still missing all the man files the Makefile.am references, thus I hacked the build to not enter the man/ directory at all. This had been mentioned above too, even it's not directly related to the static analysers.

Anyway, the build finally succeeded and here is the result: scan-results.html Change the extension to .html, the GitHub doesn't allow attaching .html files... :-/

[mcrha]

I forgot to mention, the package name contains the date and the commit hash it had been built from. Feel free to ask for more builds, it will be simpler now.

[pabs3]

@mcrha made a bunch of fixes, could you update the scan?

[mcrha]

Sure thing. Here you are.