xfs
[Top] [All Lists]

Re: [PATCH] xfs: fix possible NULL dereference

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix possible NULL dereference
From: Geyslan GregÃrio Bem <geyslan@xxxxxxxxx>
Date: Wed, 23 Oct 2013 08:58:51 -0200
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, Ben Myers <bpm@xxxxxxx>, Alex Elder <elder@xxxxxxxxxx>, open list <linux-kernel@xxxxxxxxxxxxxxx>, XFS FILESYSTEM <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=gfV3a10dSL3F51NCnlzsYLtxzCUeBFmULAF5F4Txxr4=; b=j6HvIpGlpHN+LtClArkAHDuTM6yxqaE4jCJW/Y0QM+0OY2YU9aw91zDoxEYO1c4j62 AO1tnqDe1CDL0YiYtKGpmnQLvluTVSm4dfFnuhzSBovlNN3EkzLuqgnDkuZjHHrs/vMc b1it0pOp6ENsHYz+SjS82H9HQii0HCi042pIQOzL4pud3OYVBeb9kNc5rYrcAHXLvmCC OvZ6Gm/n52cM0OEKkXmioyaC0mvStXMulguJkeAkGqFtkcGS1ATcCiX1C3rhqKDSNDjK 84uQtSR+u/PdcVykxUcWq8mGTYJdu2jxHiA9abgHUSdfeacE2mEzYDN5RCBeV4pGRlGS 8J1g==
In-reply-to: <5266EBF0.901@xxxxxxxxxxx>
References: <5265956F.4010700@xxxxxxxxxxx> <20131021224459.GE16161@dastard> <5265B4D2.3000907@xxxxxxxxxxx> <20131021231849.GL10553@xxxxxxx> <20131021235601.GG4446@dastard> <5265C03B.50701@xxxxxxxxxxx> <20131022001732.GI4446@dastard> <CAGG-pUTh-PJJ4Nzo0r-f3VDPMc81U2z_NMX+Wcex3KzGs=U8cA@xxxxxxxxxxxxxx> <20131022203946.GB2797@dastard> <5266E4BD.8030601@xxxxxxxxxxx> <20131022210300.GC2797@dastard> <5266EBF0.901@xxxxxxxxxxx>
2013/10/22 Eric Sandeen <sandeen@xxxxxxxxxxx>:
> On 10/22/13 4:03 PM, Dave Chinner wrote:
>> On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
>>> On 10/22/13 3:39 PM, Dave Chinner wrote:
>>>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan GregÃrio Bem wrote:
>>>>> 2013/10/21 Dave Chinner <david@xxxxxxxxxxxxx>:
>>>>>> On Mon, Oct 21, 2013 at 07:00:59PM -0500, Eric Sandeen wrote:
>>>>>>> On 10/21/13 6:56 PM, Dave Chinner wrote:
>>>>>>>> On Mon, Oct 21, 2013 at 06:18:49PM -0500, Ben Myers wrote:
>>>>>>
>>>>>> Yes, but to continue the Devil's Advocate argument, the purpose of
>>>>>> debug code isn't to enlightent the casual reader or drive-by
>>>>>> patchers - it's to make life easier for people who actually spend
>>>>>> time debugging the code. And the people who need the debug code
>>>>>> are expected to understand why an ASSERT is not necessary. :)
>>>>>>
>>>>> Dave, Eric and Ben,
>>>>>
>>>>> This was catched by coverity (CID 102348).
>>>>
>>>> You should have put that in the patch description.
>>>>
>>>> Now I understand why there's been a sudden surge of irrelevant one
>>>> line changes from random people that have never touched XFS before.
>>>>
>>>> <sigh>
>>>>
>>>> Ok, lets churn the code just to shut the stupid checker up. This
>>>> doesn't fix a bug, it doesn't change behaviour, it just makes
>>>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
>>>> in a previous message.
>>>
>>> You know, I respectfully disagree, but we might just have to agree
>>> to disagree.  The code, as it stands, tests for a null ptr
>>> and then dereferences it.  That's always going to raise some
>>> eyebrows, coverity or not, debug code or not, drive by or not.
>>
>>> So even for future developers, making the code more self-
>>> documenting about this behavior would be a plus, whether it's by
>>> comment, by explicit ASSERT(), or whatever.  (I don't think
>>> that xfs_emerg() has quite enough context to make it obvious.)
>>
>> Sure, but if weren't for the fact that Coverity warned about it,
>> nobody other that us people who work on the XFS code day in, day out
>> would have even cared about it.
>>
>> That's kind of my point - again, as the Devil's Advocate - that
>> coverity is encouraging drive-by "fixes" by people who don't
>> actually understand any of the context, history and/or culture
>> surrounding the code being modified.
>
Dave, If Coverity had not caught this, I could have never sent a patch
to xfs in my entire life.
So, I need not to know the xfs culture, code or context to identify a
flagrant, intentional or not, code that seems a bug.
Open source world works this way, all can help, but only a few decide
to do it. And there many ways to do it; static analysis is only one.

> They shouldn't have to, the code (or comments therein) should
> make it obvious.  ;)  (in a perfect world...)
>
>> I have no problems with real bugs being fixed, but if we are
>> modifying code for no gain other than closing "coverity doesn't like
>> it" bugs, then we *should* be questioning whether the change is
>> really necessary.
>
> But let's give Geyslan the benefit of the doubt, and realize that
> Coverity does find real things, and even if it originated w/ a
> Coverity CID, when one sees:
>
>         if (!a)
>                 printk("a thing\n")
>
>         a = a->b = . . .
>
> it looks suspicious to pretty much anyone.  I don't think Geyslan
> sent it to shut Coverity up, he sent it because it looked like
> a bug worth fixing (after Coverity spotted it).
>
Eric, you're right. In this particular case, I didn't send to shut Coverity up.
And yeah, it looked like a bug that worth to fixing.

> Let's not be too hard on him for trying; I appreciate it more
> than spelling fixes and whitespace cleanups.  ;)
>
> I agree that some Coverity CIDs are false, and we shouldn't
> mangle code just to make it happy, but I just don't think that's
> what's going on here.  Let's imagine Geyslan saw 10 other CIDs
> and elected not to send changes, because they didn't look like
> they needed fixing, but this one looked like a bona fide bug.
>
Yep.

>> Asking the question may not change the outcome, but we need to ask
>> and answer the question regardless.
>
>>> (We don't have to change code to shut up coverity; we can flag
>>> it in the database and nobody else will see it.)
>>
>> Only if you are the first to see it and make an executive decision
>> that it's not necessary to fix.... :/
>
> Or, you find it, send a patch that seems reasonable, get massive
> pushback, (hopefully) flag it, and resolve never come back to xfs
> again.  ;)
Really, FWIW, the whole discussion to me is somewhat prolix. Let's see:

- We have a Coverity spot (Dereference after NULL check) = BUG.
- You identified that was intentional and isn't a bug. Ok?
- Regarding the "possible new patch" subject, I humbly pass the ball to you.

Thank you for the attention.

Geyslan
Regards.
>
> -Eric
>
>> Cheers,
>>
>> Dave.
>>
>

<Prev in Thread] Current Thread [Next in Thread>