Check static analyzer log #6
Comments
|
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/
|
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.
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. |
The DEBUG_WARN has two parameters, so it should have two conversions. See-also: #6
|
@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 |
|
I've a hard time to build the branch under Fedora: 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. |
|
There is also missing content of the |
|
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/
|
|
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/
|
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. |
|
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/
|
|
@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? |
|
You do not need to mention me, I'm still here :)
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). |
|
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/
|
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 a3c033a is here: |
The existing calls often used incorrect specifiers for the argument types. The PRIx64 style specifiers and z length modifiers for uint64_t style and size_t integer types are portable between architectures while manual length modifiers are not. Drop casts from arguments where a length modifier should be used instead. Add casts to arguments where needed to change types to what printf expects. Correct types or signedness of variables where the printf specifier is better. Switch from printing pointers as integers to printing them as pointers. Also ensure a space is present before and after PRIx64 style specifiers, for compatibility with C++11. Suggested-by: https://github.com/csutils/csmock See-also: pst-format#6
|
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. |
|
An updated scan-results.html is here. It's at commit 16ca280 . 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 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. |
The existing calls often used incorrect specifiers for the argument types. The PRIx64 style specifiers and z length modifiers for uint64_t style and size_t integer types are portable between architectures while manual length modifiers are not. Drop casts from arguments where a length modifier should be used instead. Add casts to arguments where needed to change types to what printf expects. Correct types or signedness of variables where the printf specifier is better. Switch from printing pointers as integers to printing them as pointers. Also ensure a space is present before and after PRIx64 style specifiers, for compatibility with C++11. Suggested-by: https://github.com/csutils/csmock See-also: pst-format#6
The existing calls often used incorrect specifiers for the argument types. The PRIx64 style specifiers and z length modifiers for uint64_t style and size_t integer types are portable between architectures while manual length modifiers are not. Drop casts from arguments where a length modifier should be used instead. Add casts to arguments where needed to change types to what printf expects. Correct types or signedness of variables where the printf specifier is better. Switch from printing pointers as integers to printing them as pointers. Also ensure a space is present before and after PRIx64 style specifiers, for compatibility with C++11. Suggested-by: https://github.com/csutils/csmock See-also: pst-format#6
|
@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? |
The existing calls often used incorrect specifiers for the argument types. The PRIx64 style specifiers and z length modifiers for uint64_t style and size_t integer types are portable between architectures while manual length modifiers are not. Drop casts from arguments where a length modifier should be used instead. Add casts to arguments where needed to change types to what printf expects. Correct types or signedness of variables when printf specifier is better. Switch from printing pointers as integers to printing them as pointers. Drop a format string macro that was used with two different types. Also ensure a space is present before and after PRIx64 style specifiers, which are macros, for readability and compatibility with C++11. Suggested-by: https://github.com/csutils/csmock See-also: #6
|
I have rechecked, fixed and merged the patch fixing the printf format strings. |
|
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). |
|
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/
|
|
Sure, I can help. It's still kinda tricky to make it build from the git snapshot, because the Anyway, the build finally succeeded and here is the result: |
|
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. |
Suggested-by: gcc -Wanalyzer Suggested-by: https://github.com/csutils/csmock See-also: #6
The fallthrough is intended but the code is clearer without it. Suggested-by: https://github.com/csutils/csmock See-also: #6
open() returns -1 to indicate an error, the code checked for 0 for errors. Suggested-by: https://github.com/csutils/csmock See-also: #6
Suggested-by: https://github.com/csutils/csmock See-also: #6
snprintf takes a char* but the var was a uint8_t, which is unsigned char*. Suggested-by: https://github.com/csutils/csmock See-also: #6
snprintf takes a char* but the var was a uint8_t, which is unsigned char*. Suggested-by: gcc -Wpointer-sign Suggested-by: https://github.com/csutils/csmock See-also: #6
snprintf takes a char* but the var was a uint8_t, which is unsigned char*. Suggested-by: gcc -Wpointer-sign Suggested-by: https://github.com/csutils/csmock See-also: #6
The move is more efficient than the copies. Suggested-by: https://github.com/csutils/csmock See-also: #6
Suggested-by: https://github.com/csutils/csmock See-also: pst-format#6
|
@mcrha made a bunch of fixes, could you update the scan? |
|
Sure thing. Here you are. |
Switch away from using function-scope static variables, because there is no easy way to free them. Suggested-by: valgrind See-also: #6
Suggested-by: https://github.com/csutils/csmock See-also: pst-format#6
Suggested-by: https://github.com/csutils/csmock See-also: pst-format#6
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].The text was updated successfully, but these errors were encountered: