On 19-10-2014 21:12, Jon A. Cruz wrote:

Hi,

I've been working with Johan and Bryce on the clang and Coverity analysis. Bryce should be getting the coverity runs more automated soon, but we'll take a little before things get published. We'll need to ensure we are sensitive to security, disclosure, etc.

However I do have a few preliminary results. First goes to leveraging the clang results effectively. We'll need to pull together some wiki info on which clang issues to prioritize, and what approaches might be used for remediation of some of the reported problems. Initially we have one question on "Dead assignment" and the other dead store. The first case Johan and I looked at was one that did warrant skipping initialization... however clang reports 167 of those while Coverity lists only 8 issues that might overlap them. So this set of clang warnings appears to be a bit subjective on the clang dev's part, or perhaps just more advisory. So we should probably look at addressing those last.

I agree. We can disable the "Dead assignment" analyzer output for now, and enable it again when we want it back. I do think there is value in the warning, but it is certainly not a priority.

In the Coverity results I spotted two different big-picture issues hiding in the details. One warrants just low-level code cleanup while the other should be looked over by as many people keeping 'architecture' in mind.

The simple issue is with the GTK-style casting macros such as SP_IS_USE(x) and SP_USE(x). They started out following the GTK team's approach to adding objects to C. A while back these were refactored to be C++ oriented so existing code would not have to be changed as we migrated to true classes. However, the usage is backwards (check-then-cast vs. cast-then-check) and conceptually there are some big mismatches that lead to most usages being vulnerable to null pointer dereferencing. Conceptually these are also backwards as they use run-time checks instead of compile-time checks.

...

From an implementation viewpoint, these macros have two major problems.

First is that they cast away any const attributes and thus make the code less safe. Secondly (and far more critical) they are using C-Style casts that also make them take any random block of data and treat it as a proper C++ object. One of my test cases was to feed the macros a simple array of 5 arbitrary bytes. I expected it to accept but result in NULL, however it instead gave me a seemingly valid SPUse pointer. Not good.

The good news is that in the vast majority of cases, no macros or casts are needed at all in C++ (for upcasting, etc.) and thus their use can just be removed. I'm working through an initial cleanup pass that should correct all but those where the improper loss of 'const' comes into play. This cleanup is good since these macros are responsible for 600-800 of the outstanding 1,135 Coverity defects.

Replacing GSList usage with STL containers should get rid of a bunch of static_casts from void*, too.

The second issue is a higher-level problem of design and naming. There are a few methods who's names appear to be leading to buggy use. The main example is SPShape::getCurve(). The problem is that to convey proper meaning this probably should be called something more like SPShape::generateNewCurve(). Many developers were treating it similar to std::string::c_str() and accessing it to query attributes of the resulting curve without explicitly grabbing and then freeing the curve. That is, the name implied access to an existing object instead of creation and ownership transfer of a new object.

To fix this we might simply change the naming approach (e.g. avoid 'getFoo()' for anything that allocates) and the using code... or we could change getCurve() and friends to operate on an internal object that is persistent... or perhaps something else.

I like the name change. Perhaps a change of interface itself would be good to. E.g., when getCurve() would not create a new object, return a Curve& instead of * ? And for the creation functions, return a std::unique_ptr<Curve> ? (allowing "shape.generateNewCurve()->doSmth();" without memleak) I think the interface of SPShape::setCurveInsync is also very easy to misuse, and could use some improvement.

2 related remarks: - For any refactoring, please separate it from whitespacing or trivial noop commits. It makes code review much harder when the commit diff is huge and "littered" with spacing improvements, removal of "this->", adding of { }, reduce variable scope, etc. - I hope to get automated render tests running again soon, so we can feel more confident about the refactoring not breaking something unexpectedly, keeping trunk closer to a releasable state.

thanks so much for any work in this area, Johan