From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 In-Reply-To: <241575e8d4de5ee06f9a745cd4056fc4@felloff.net> References: <241575e8d4de5ee06f9a745cd4056fc4@felloff.net> From: Giacomo Tesio Date: Wed, 18 Jan 2017 10:24:00 +0100 Message-ID: To: Fans of the OS Plan 9 from Bell Labs <9fans@9fans.net> Content-Type: multipart/alternative; boundary=001a1137d29226dd2905465afa07 Subject: Re: [9fans] memory leaks in libmp Topicbox-Message-UUID: b1eb3b5e-ead9-11e9-9d60-3106f5b1d025 --001a1137d29226dd2905465afa07 Content-Type: text/plain; charset=UTF-8 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 : > > 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 > > --001a1137d29226dd2905465afa07 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Oh, well, I'm sorry, I should have clar= ified the context a bit more. Here it is.

The link I provided here a= re 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 hi= nt.

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

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

Gi= ven that, I thought: why not share these with 9fans and 9front since they u= se that code too but cannot access coverity?

Now, for Jehanne these = issues were already marked as "worth considering" (and in this ca= se "worth fixing").
And I'm fine if you consider them fals= e 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 programmer= s than me.
Indeed I really like your feedbacks, because you are very dee= p in the code base and it usually take you seconds to understand issues tha= t require me days to figure out.


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


As you note, I could do it test-driven, providin= g 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, unfor= tunately, I'm working alone and I do not have the energy to do this eve= ry 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 ful= l one.

Thus I cannot share patches that you can blindly a= pply, sorry.


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

To my eye it sh= ould only consider bufp if buf is nil. And bufp should be not nil in that c= ase.

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

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

= =C2=A0 =C2=A0 =C2=A0mptole(num, s->buf, s->len, nil)

it will c= ause the leak if the struct was just zeroed.

In this case I prefer t= he assert fail to the leak, so that I, as a dumb guy, would notice the issu= e more rapidly.


Giacomo


=

2017-01-18 3:31 GMT+01:00 <cinap_lenrek@felloff.net>= :
> Still a lo= t of coverity defects are actually false positives.
> I try to signal here only the actual bugs but I might be wrong, thus l= et me
> know if you find these report useless and I will stop to annoy the lis= t.

i cannot predict the future nor tell you how to spend your time. i&#= 39;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<= br> wastes our time when you do not do this), and not just blindly present the<= br> coverty output as a proof.

--
cinap


--001a1137d29226dd2905465afa07--