[Nut-upsdev] [nut-commits] svn commit r3383 - branches/coverity

[Arnaud Quette]

Hi Michal,

> >     Log:
> > >     Creating a branch for Coverity reported problems
> >
> >
> > I'm very interested there!
> > Have you been able to get NUT part of the Coverity Scan program, or are
> > you running Coverity on your own?
>
> we have bought Coverity license, so I've checked it on my own.


cool ^_^
I appreciate a lot, since this is a task, sweet to my heart, that has been
neglected for years.
I've btw affected you the related task on Alioth...

> I'd love to see a summary report, to get an overall idea of the situation.
> >
> > cheers,
> > Arnaud
>
> I was thinking what's the best way to send you several patches. I think
> new branch is the easiest way to do so.
>

it's indeed a good way!

I've parsed Coverity scan result, removed false positives and fixed several
> issues. Not all of them are bugs, so check that branch and pick those you
> think are worthy. Coverity reports a lot of issues that are only "not that
> pretty code, but not worth to change it". So it's always a question where
> you should draw the line :)
>
> I've fixed everything I've found worth to fix, except 2 issues I don't
> know whether/how to fix. There are also a lot of reports about sprintf
> (Coverity reports every sprintf as "possibly dangerous", but hard to tell
> if it's worth it to fix it.


the possibly dangerous nature is quite probably due to the possibility of
overflow of the target buffer.
the simplest fix there is to replace sprintf calls by snprintf, and add the
max size of the container.


> I've checked all of them if there was any obvious issue.)
>
> The two issues I did not fix:
> 1) Error: REVERSE_INULL:
> clients/upsmon.c:789: deref_ptr_in_call: Dereferencing pointer "un". (The
> dereference is assumed on the basis of the 'nonnull' parameter attribute.)
> clients/upsmon.c:795: check_after_deref: Dereferencing "un" before a null
> check.
>
> I don't know if 'un' can be null, but it is checked for null after strcmp,
> if it can really be null, it should be checked before strcmp
>

"un" is the username inherited from upsmon.con->MONITOR line(s)
all fields in this line are mandatory.
note that, historically, username was not mandatory.
no, it's mandatory, so the logic has changed, but not all the code.

there, prior to calling redefine_ups() (in addups()), all fields are
checked (including username 'un').
if 1 is NULL, the line is ignored.

thus, we can safely consider 'un' to be non NULL in redefine_ups()
I've fixed it in r3557.

2) Error: CONSTANT_EXPRESSION_RESULT: (4x)
> drivers/libshut.c:731: result_independent_of_**operands: (Start[0] &
> 0x80) == 5 is always false regardless of the values of its operands. This
> occurs as the logical operand of if.
>
> and exactly the same thing at
> libshut.c:973,
> mge-shut.c:527,
> mge-shut.c:839,
>
> in libshut.c and mge-shut.h, there are
> #define SHUT_TYPE_NOTIFY        0x05
> #define SHUT_PKT_LAST           0x80
>
> so if ((c & SHUT_PKT_LAST) == SHUT_TYPE_NOTIFY) can't be true
> (c & 0x80) == 0x05
>

waow, the developer (sigh) was indeed brain damaged when he (I) did that ;-)
fixed in r3556


> And that's all for first round :) Let me know if you have any question.
>

very nice, thanks a lot ;-)

I've just merged your branch into the trunk (r3555).

cheers,
Arnaud
-- 
Linux / Unix Expert R&D - Eaton - http://powerquality.eaton.com
Network UPS Tools (NUT) Project Leader - http://www.networkupstools.org/
Debian Developer - http://www.debian.org
Free Software Developer - http://arnaud.quette.free.fr/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20120503/e250752a/attachment.html>