Re: Static code analysis of Octave

[rik]

On 08/14/2015 05:05 AM, address@hidden wrote:

Subject:

Re: Octave has been checked by PVS-Studio

From:

Juan Pablo Carbajal <address@hidden>

Date:

08/13/2015 12:57 PM

To:

Игорь Штукарев <address@hidden>

CC:

Maintainers GNU Octave <address@hidden>

List-Post:

<mailto:address@hidden>

Content-Transfer-Encoding:

quoted-printable

Precedence:

list

MIME-Version:

1.0

References:

<address@hidden> <address@hidden>

In-Reply-To:

<address@hidden>

Message-ID:

<address@hidden>

Content-Type:

text/plain; charset=UTF-8

Message:

5

On Thu, Aug 13, 2015 at 9:53 PM, Juan Pablo Carbajal
<address@hidden> wrote:

> On Thu, Aug 13, 2015 at 4:43 PM, Игорь Штукарев <address@hidden> wrote:

>> Hi, i am taking internship in the company that develops static analyzer
>> PVS-Studio. One of my tasks was to scan Octave with analyzer and writing an
>> article about that.  You can find some results of general analysis there. To
>> get full list of warnings you can use PVS-Studio. Best regards, Igor
>> Shtukarev.

>
> Probably you wanted to refer to "Matlab" not "Mathcad". Mathcad is
> oriented to symbolic calculations and typesetting.

There are a couple of interesting findings! I hope this is useful.

8/14/15

Igor,

It's always good to get an outside analysis of things.  However, the code can be very complex which can fool some analyzers.  One doesn't try to write complex code, but sometimes one is forced to.

The example in mex.cc is one of those.  Octave is required to support the MeX interface from Matlab (a 'C' API supported in C++).  The global variable mex_context is used not in the local function, but in other functions that may be invoked through call_mex().  For the life of the call_mex function the global value assumes a certain value.  At the end of the function the original global value is restored by an unwind protect frame object and all is well.

The example in tmpdir.c does not belong to Octave.  Octave uses gnulib to get access to basic cross-platform functions like tmpdir().  This does look like a problem which you might want to report on the gnulib site.  It only affects Windows platforms.

There isn't any problem in colamd.cc.  The Pinv pointer is created by the OCTAVE_LOCAL_BUFFER macro which expands out around a trivial class to create a pointer to memory which is reliably cleaned up.  The internal class for octave_local_buffer uses C++ new/delete mechanisms so an exception would have been thrown earlier in the code if memory was not available.

The usage of

instance = new XXX
if (instance)
 ...

is a relic of bad compilers.  However, it's not even as bad as it looks.  I count 34 examples of this usage in liboctave and libinterp and these are all for singleton items that are created once when Octave starts, but never again.  That's a pretty small overhead.

I agree that something looks wrong with cellfun.cc.  It seems like the second is_object branch should be deleted since it is never operative.

The code in kpse.cc is ancient and inherited.  I'm not surprised that there was something there.  I cleaned up the example you mentioned.

The example in load-save.cc also seems valid so I fixed that.

In oct-stream.cc, I don't see a problem with

while (is && !is.eof() ...

If you look at the C++ istream class you will find that evaluating the stream in a boolean context is the equivalent of (!is.fail && !is.bad) so we already have the equivalent of !is.fail in the while loop conditional.

In Sparse.cc the code looks wrong, but in fact is just complex.  Octave stores sparse matrices in a compressed column format and

      for (i = i; i < u; i++)

is exactly needed to determine how many elements are in this particular column.

I made the changes that looked good on the development branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/610c74748518).

Thanks for reviewing Octave.

--Rik