Re: Static analyzer run

[Bill Allombert <Bill.Allombert@math.u-bordeaux1.fr>]

On Mon, Sep 14, 2009 at 10:00:32AM +0200, Lorenz Minder wrote:
> Hi,
> 
> I've run PARI through a static source code analyzer to see if it finds
> any bugs (and to see if the tool is worth anything).
> 
> It found a couple of minor things, such as duplicate assignments.  I
> think the benefit of fixing those is mostly code readability.  Those
> are in the attached minor_bugs.patch.

I have commited your patch with very small changes.

> Then it uncovered a few real bugs. I (hope I) fixed those in
> sa_bugfixes.patch.

Also done.

> All tests still run fine with those patches attached.
> 
> And then it gave a couple of messages I did not investigate closer,
> but that look very suspicious.  I think it would probably be a
> worthwhile time investment if some PARI guru looked into those.

Can you tell the analyzer that pari_err does not return ?
This is a large source of false positive with gcc.
  
> ../src/basemath/base2.c:1637:16: warning: Dereference of null pointer
>       S->phi = typ(opa) == t_INT? RgX_Rg_add_shallow(S->phi, opa)
>                ^

I do not understand why it work, but clearly the statement
opa = NULL; 
is useless. Somehow the code assume that getprime will work the first
time.

> ../src/basemath/base2.c:2383:3: warning: Branch condition evaluates to an uninitialized value.
>   if (den)
>   ^   ~~~

Probably the analyzer does not know that pari_err(typeer,"Rg_to_ff");
will not return.

> ../src/basemath/base3.c:1553:7: warning: Value stored to 'ex' is never read
>       ex = EX;
>       ^    ~~
and
> ../src/basemath/base3.c:1563:16: warning: Value stored to 'ex' is never read
>         if (v) ex = mulii(ex, powiu(p, v));
>                ^    ~~~~~~~~~~~~~~~~~~~~~~

Looking at the code, one wonder why this variable ex exists at all.
Maybe the intent was 'EX = ex' instead, but that does not work.

> ../src/basemath/bibli1.c:2129:14: warning: Value stored to 'j' is never read
>         for (j = 0; i <= s; i++)
>              ^   ~

Of course this is wrong, but what is the fix ???

> ../src/basemath/polarit3.c:2257:36: warning: Dereference of null pointer
>   if (av2) { avma = av2; *lambda = next_lambda(*lambda); }
>                                    ^



> ../src/basemath/rootpol.c:2255:27: warning: Value stored to 'av2' is never read
>       xd = RgX_deriv(xc); av2 = avma;
>                           ^     ~~~~

It probably should be avma = av2 instead.

> ../src/language/eval.c:625:11: warning: Dereference of null pointer
>       if (typ(base)!=t_VEC) sbase = GSTR(base);
>           ^

This a false positive.

> ../src/modules/QX_factor.c:1147:63: warning: Pass-by-value argument in function call is undefined.
>     if (DEBUGLEVEL>5) msgtimer("gcd mod %lu (bound 2^%ld)", p,expi(q));
>                                                               ^    ~
and
> ../src/modules/QX_factor.c:1154:10: warning: Pass-by-value argument in function call is undefined.
>     qp = muliu(q,p);
>          ^     ~

This looks like a false positive.

Cheers,
Bill.