[9fans] memory leaks in libmp

[9fans] memory leaks in libmp

[Giacomo Tesio]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

Hi, these other two defects identified from Coverity scan that you might
find interesting:

https://github.com/JehanneOS/jehanne/issues/5
https://github.com/JehanneOS/jehanne/issues/6

NOTE: 9front's guys consider these a false positive, since both

    mptole(n, nil, 0, nil);

and

    mptobe(n, nil, 0, nil);

are stupid bugs in the caller.

Since, I do stupid errors often, I prefer to fix the leak.

Still a lot of coverity defects are actually false positives.
I try to signal here only the actual bugs but I might be wrong, thus let me
know if you find these report useless and I will stop to annoy the list.


Giacomo

[-- Attachment #2: Type: text/html, Size: 955 bytes --]

Re: [9fans] memory leaks in libmp

[cinap_lenrek]

> Still a lot of coverity defects are actually false positives.
> I try to signal here only the actual bugs but I might be wrong, thus let me
> know if you find these report useless and I will stop to annoy the list.

i cannot predict the future nor tell you how to spend your time. i'm *not*
claiming that using static code analysis to find bugs is "useless" per se.

but consider the context in which problems would occur, maybe even describe
how a bug would manifest itself in some code (thats what takes the time or
wastes our time when you do not do this), and not just blindly present the
coverty output as a proof.

--
cinap

Re: [9fans] memory leaks in libmp

[Giacomo Tesio]

[-- Attachment #1: Type: text/plain, Size: 3725 bytes --]

Oh, well, I'm sorry, I should have clarified the context a bit more. Here
it is.

The link I provided here are Jehanne issues, not Plan 9 or 9front bug
reports.
After fixing a few of them I realized that, an year from now, I won't be
able to remember why I did the change. Also, coverity could shut down and I
would have no hint.

So I started coping the coverity scan comments, as a sort of note to a
future self wondering "Why the hell I changed this code?".

The coverity comments in the Jehanne issues are NOT a proof of anything,
they are just quick reminders for Jehanne's developers.

Given that, I thought: why not share these with 9fans and 9front since they
use that code too but cannot access coverity?

Now, for Jehanne these issues were already marked as "worth considering"
(and in this case "worth fixing").
And I'm fine if you consider them false positive or even use a different
fix, since that force me to challenge my assumptions, to review the code
more and even learn from better programmers than me.
Indeed I really like your feedbacks, because you are very deep in the code
base and it usually take you seconds to understand issues that require me
days to figure out.


But these are still **Jehanne's issues**, they are shared in the hope they
helps but with no warranty! ;-)


As you note, I could do it test-driven, providing a failing test and then
fixing the test. You are right, it would be much more useful to Jehanne and
maybe to the Plan 9 ecosystem too.
But, unfortunately, I'm working alone and I do not have the energy to do
this every single time. And I'm one of those few weird people thinking that
you should not care about code coverage if you don't want to pay for a full
one.

Thus I cannot share patches that you can blindly apply, sorry.


As for the issue in question, I think it's a actually a bug. mp(2) states
that
>
> Mptobe and mptole convert an mpint to a byte array.  The
> former creates a big endian representation, the latter a
> little endian one.  If the destination buf is not nil, it
> specifies the buffer of length blen for the result.  If the
> representation is less than blen bytes, the rest of the
> buffer is zero filled.  If buf is nil, then a buffer is
> allocated and a pointer to it is deposited in the location
> pointed to by bufp. Sign is ignored in these conversions,
> i.e., the byte array version is always positive.

To my eye it should only consider bufp if buf is nil. And bufp should be
not nil in that case.

Thus the fix was simply to add an assert to make if fail fast instead of
leaking memory.

The simple reasoning I did during triage was: consider a pointer to a
struct holding both buf and its length:

     mptole(num, s->buf, s->len, nil)

it will cause the leak if the struct was just zeroed.

In this case I prefer the assert fail to the leak, so that I, as a dumb
guy, would notice the issue more rapidly.


Giacomo



2017-01-18 3:31 GMT+01:00 <cinap_lenrek@felloff.net>:

> > Still a lot of coverity defects are actually false positives.
> > I try to signal here only the actual bugs but I might be wrong, thus let
> me
> > know if you find these report useless and I will stop to annoy the list.
>
> i cannot predict the future nor tell you how to spend your time. i'm *not*
> claiming that using static code analysis to find bugs is "useless" per se.
>
> but consider the context in which problems would occur, maybe even describe
> how a bug would manifest itself in some code (thats what takes the time or
> wastes our time when you do not do this), and not just blindly present the
> coverty output as a proof.
>
> --
> cinap
>
>

[-- Attachment #2: Type: text/html, Size: 4383 bytes --]