Re: [PATCH v2] net: qrtr: fix null pointer dereference in qrtr_ns_remove

Re: [PATCH v2] net: qrtr: fix null pointer dereference in qrtr_ns_remove

[Qinglang Miao]

Hi Markus,

I'd like to take some of your advice in this patch, but I noticed that 
this one has been applied.

Some of your advice would be considered kindly on my future work.

Thanks.

在 2021/1/5 21:14, Markus Elfring 写道:
>> A null-ptr-deref bug is reported by Hulk Robot like this:
> 
> Can it be clearer to use the term “null pointer dereference” for the final commit message?
This advice is too detailed for 'null-ptr-deref' is known as a general 
phrase like 'use-after-free' for kernel developer, I think.>
> 
>> --------------
> 
> I suggest to choose an other character for drawing such a text line.
It's an acceptable advice, thanks.
> 
> 
>> Fix it by making …
> 
> Would you like to replace this wording by the tag “Fixes”?
Sorry, I didn't get your words.

'Fix it by' follows the solution
'Fixes' follows the commit which brought the problem.

In fact, I do considered using 'Fixes' on this one, but it's hard to 
tell which specific commit brought this null pointer dereference.
> 
> Will an other imperative wording variant be helpful for this change description?
> 
> 
> …
>> +++ b/net/qrtr/qrtr.c
>> @@ -1287,13 +1287,19 @@ static int __init qrtr_proto_init(void)
> …
>> +err_sock:
>> +	sock_unregister(qrtr_family.family);
>> +err_proto:
>> +	proto_unregister(&qrtr_proto);
>>   	return rc;
>>   }
> 
> Would it be clearer to use the labels “unregister_sock” and “unregister_proto”?
In fact, The reason I use 'err_sock' rather than 'unregister_sock' is to 
keep same in 'net/qrtr/ns.c'.

I agree with you that “unregister_sock” is better in normal case.
> 
> Regards,
> Markus
> .
> 

Re: [v2] net: qrtr: fix null pointer dereference in qrtr_ns_remove

[Qinglang Miao]

在 2021/1/6 16:09, Markus Elfring 写道:
>>>> A null-ptr-deref bug is reported by Hulk Robot like this:
>>>
>>> Can it be clearer to use the term “null pointer dereference” for the final commit message?
>> This advice is too detailed for 'null-ptr-deref' is known as a general phrase
> 
> This key word was provided already by the referenced KASAN report.
>
Yep, you're right. 'null-ptr-deref' is not really proper here.
> 
>> like 'use-after-free' for kernel developer, I think.
> I suggest to reconsider the use of abbreviations at some places.
>  >
>>>> Fix it by making …
>>>
>>> Would you like to replace this wording by the tag “Fixes”?
>> Sorry, I didn't get your words.
>>
>> 'Fix it by' follows the solution
> 
> I propose to specify the desired adjustments without such a prefix
> in the change description.
Sorry, I can understand what you means, but I still disagree with this 
one, for:

1. 'Fix it by' is everywhere in kernel commit message.
2. I think adding it or not makes no change for understanding.
3. I'm not sure this is an official proposal.

> 
> 
>> In fact, I do considered using 'Fixes' on this one,
> 
> Thanks for such information.
> 
> 
>> but it's hard to tell which specific commit brought this null pointer dereference.
> 
> This aspect is unfortunate here. >
> Regards,
> Markus
> .
> 

Thanks anyway, I shall pay more attention to commit message. ;D

Re: [v2] net: qrtr: fix null pointer dereference in qrtr_ns_remove

[Dan Carpenter]

On Wed, Jan 06, 2021 at 05:46:22PM +0800, Qinglang Miao wrote:
> 
> 
> 在 2021/1/6 16:09, Markus Elfring 写道:
> > > > > A null-ptr-deref bug is reported by Hulk Robot like this:
> > > > 
> > > > Can it be clearer to use the term “null pointer dereference” for the final commit message?
> > > This advice is too detailed for 'null-ptr-deref' is known as a general phrase
> > 
> > This key word was provided already by the referenced KASAN report.
> > 
> Yep, you're right. 'null-ptr-deref' is not really proper here.
> > 
> > > like 'use-after-free' for kernel developer, I think.
> > I suggest to reconsider the use of abbreviations at some places.
> >  >
> > > > > Fix it by making …
> > > > 
> > > > Would you like to replace this wording by the tag “Fixes”?
> > > Sorry, I didn't get your words.
> > > 
> > > 'Fix it by' follows the solution
> > 
> > I propose to specify the desired adjustments without such a prefix
> > in the change description.
> Sorry, I can understand what you means, but I still disagree with this one,
> for:
> 
> 1. 'Fix it by' is everywhere in kernel commit message.
> 2. I think adding it or not makes no change for understanding.
> 3. I'm not sure this is an official proposal.
> 

Feel free to ignore Markus...  :/  We have asked him over and over to
stop sending these sort of advice but he refused and eventually he was
banned from the mailing lists.  The rest of us can't see his messages to
you unless we're included personally in the CC list.

regards,
dan carpenter