CodeSonar reports against logog

[pas...@gmail.com]

I recently integrated logog as a test into one of my projects which was analyzed using CodeSonar (static analysis tool).  The attached is a list of the issue reports that I thought someone might want to look into.  I haven't taken enough time to familiarize myself with the internals of the library to suggest/make changes on my own but thought the information may still be useful.

Paul.

[John Byrd]

Thanks for submitting this, but I'm not seeing much useful information
in this report. It's flagging a lot of spurious errors in the Mutex
section for example.

I clean compile logog with /W4 under the Microsoft toolchain and with
-Wall under gcc. Additionally it compiles clean with Microsoft Visual
Studio 2012's Code Analysis with default settings, as well as a
relatively recent build of PVS-Studio, which I consider to be the gold
standard for static analysis.

If there are serious problems in the code, I don't think automated
analysis will catch them at this point.

> --
> You received this message because you are subscribed to the Google Groups
> "Logog Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to logog-devel...@googlegroups.com.
> To post to this group, send email to logog...@googlegroups.com.
> Visit this group at http://groups.google.com/group/logog-devel.
> For more options, visit https://groups.google.com/groups/opt_out.



--
---

John Byrd
Gigantic Software
2102 Business Center Drive
Suite 210-D
Irvine, CA 92612-1001
http://www.giganticsoftware.com
T: (949) 892-3526 F: (206) 309-0850

[pas...@gmail.com]

John:

I finally did take some time to review the reports and while I agree there are many false positives reported, there is some goodness in there, too.  My analysis of the reports follows, ordered in least to most important.

1 - Reports from test.cpp and unittest.cpp

These were not reviewed due to being test code

2 - All items in mutex.cpp are false positives

CodeSonar didn't deal with the ScopedLock class correctly and incorrectly identified double-lock and unlocks

3 - All reports against LogFile::WriteUnicodeBOM (target.cpp) are valid but we don't care

Some form of these will be reported based on what platform the analysis is performed on

4 - Report against LogFile::Open (target.cpp) is valid but we don't care

In this scenario and usage the race condition doesn't matter

5 - Reports against String::assign (lstring.cpp) are valid and related

@167 we assign bSign = value and @169 we re-assign bSign = number

Since at this point number == value, we can essentially remove the assignment from within the conditional @169 and both issues will be resolved.

We could @167 assign bSign = number to keep the same association that the original code had if that were important.

While a minor issue, I think it would be best to remove the side-effect assignment within the conditional.

6 - Report against Topic::Send (topic.cpp) is valid

The condition being tested will always be non-null since pCurrentNode is guaranteed non-NULL at this point of execution and since we're not doing a dynamic_cast to a Topic, value can never be non-NULL thus the condition will always evaluate to true.

A minor issue, easy enough to remove the if-check.

7 - Report against TimeStamp::Get (formatter.cpp) is valid

While it is likely very rare, localtime() can return NULL and if it does we will attempt to dereference it later and likely crash.  It would be best to put in a check of the return from localtime() and do something sensible to avoid its later usage.

8 - Reports against String::format_va are false positives but lead to a greater issue

Ignoring what the reports are actually indicating, what they're dealing with is the return value from vsnprinf(). The concern here is that the logic coded for dealing with truncated output is only valid when running against Microsoft CRT.  A POSIX implementation (such as used by gcc) has different return semantics for vsnprintf() which leads the code as implemented to be incorrect for all supported platforms.

My references for behavior are:

• Microsoft: http://msdn.microsoft.com/en-us/library/1kt27hek.aspx
  
• OpenGroup: http://pubs.opengroup.org/onlinepubs/9699919799/functions/vsnprintf.html (which will lead you here)

Microsoft will return (-1) in an overflow AND error (in which errno would also be set).  However, a POSIX implementation will only return a negative value (not just -1) on error and set errno ... and in an overflow situation, it will still return a positive number indicating the number of bytes that would've been written and it is up to the caller to detect the overflow (though only number number of bytes specified will be written).

Given these descriptions, the code is likely to fail in the error conditions it is intending guard against when running a non-Microsoft (or compatible) CRT.

Paul.

[John Byrd]

Wow. What a thorough and careful analysis of code that I'm not sure
warrants it. :) Let me try to be worth your analysis.

Issues 1-4. Deferred.

Issue 5. Fixed the conditional as you recommended.

Issue 6. This check should always fail unless someone has secretly
deleted a node. I'm not sure whether to leave it in out of an
abundance of paranoia or to flag it as a proper error condition.

Issue 7. Added check for NULL return from localtime().

Issue 8. I followed the POSIX conventions in the link a little more
closely and saw that POSIX implementations will return negative
values, not just -1, on failure to allocate. I rejiggered the
format_va function to hopefully work around this scenario.

Anyway, if you would do me the kind favor of getting the latest and
reviewing manually, I'd appreciate it.

On a totally unrelated note... what are you doing with this code?

Thanks again for the helpful analyses.

[pas...@gmail.com]

John:

I just got a copy logog-master to look at the changes.  The updates to the sources seem fine, however the updates to cmake configuration causes the library to fail to compile for me due to changing reporting of warnings to errors:

packages/logog-master/src/target.cpp: In member function ‘virtual int logog::LogFile::Open()’:
packages/logog-master/src/target.cpp:121:8: error: variable ‘bFileAlreadyExists’ set but not used [-Werror=unused-but-set-variable]

This results from my not building LOGOG_UNICODE.

For what I'm doing with the code, well that's different from what I started out doing with it.

Originally, I needed to output some simulation data when running on target hardware so it could then be analyzed and "verified." I thought the easiest way to do that would be to send the data to a remote syslog server.  I expected to implement something and then remove it once done and so I was looking for an easy to integrate logging library that either already had syslog support or one where I could easily implement such a target. Easy integration essentially means no real external dependencies to speak of and an easy to learn API so that I could implement the syslog support (if it didn't already have it).

Of course what I consider needed for verification was very different from what the person doing the verification actually needed. So even though I wrote a simple UDP- and TCP-based syslog message target, the solution was ultimately not used.  I did, however, decide to keep the logog integration for other eval purposes.

Now, I'm looking at logog as the basis of developer/development logging. Not logging in a production system, but only during development and debugging and in production all the logging statements would be "removed."  Currently, developers either write some of their own logging macros and classes or simply log directly to cout, cerr, or files for what they need. It's generally a mess and there's no utility that can be generally shared between projects and the implementations are not really extensible enough to be useful to more than one or two people.

So I thought logog might be a fine basis for common, development-only, logging support that could be easily dropped into a project needing one-time integration or the support could be generally adopted.  So far, logog is meeting my expectations for this usage; however, projects use a fair amount of std::string and boost::format objects that are a bit of a pain to use with logog.  Glog is also an option for this but has a slightly approach than logog and I'm not convinced it meets everything I would be looking for either...but then isn't that the case for every logging library?

Paul.