Re: [PATCH v4] staging: wlags49_h2: strncpy, need checking the memory length

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/15/2013 08:06 PM, Dan Carpenter wrote:
> Please read my first email again.  Pretty much everything I wanted
> to say is there.
> 
> http://driverdev.linuxdriverproject.org/pipermail/devel/2013-May/038083.html
> 

Oh, sorry, it is my fault.

Originally, I only read the upper half of the mail, did not see the
bottom half. I should notice next time.

And that mail is really complete.


> On Wed, May 15, 2013 at 06:09:14PM +0800, Chen Gang wrote:
>> > If the type of 'probe_rsp->rawData[]' is changed to 'char', it will
>> > compare between 'char' and 'int', in this condition, casting to 'int' is
>> > the normal way, all things are OK.
> You mean "unsigned char" instead of "char"...  Signed char would be
> a bug.
> 
>> > 
>> > If use min_t(u8..., we have to look up the 'HCF_MAX_NAME_LEN' whether is
>> > changed upper than 255.
> No.
> 1) I would assume the programmer had considered that.
> 2) That kind of a bug would almost certainly be caught in testing.
> 3) How many values with a name like HCF_MAX_NAME_LEN are higher than
>    255?  Names are normally are 16-32 characters long.
> 
> When I am reviewing and I see min_t():
> 1) I assume that the two types are different and that the compiler
>    warned about it.
> 2) I assume that HCF_MAX_NAME_LEN is an int so
>    "HCF_MAX_NAME_LEN - 1" is also an int.
> 
> When I see "min_t(int, XXX, HCF_MAX_NAME_LEN - 1)" then I think:
> 
> 1) I know that "HCF_MAX_NAME_LEN - 1" is int, but I don't know the
>    type for "XXX".
> 2) Are negative values for XXX valid?
> 3) Did the author stop to consider that high values can be cast to
>    negative?
> 4) A lot of people just use "int" without even considering if it's
>    appropriate.  For example, here is a real bug I fixed last week:
>    http://marc.info/?l=linux-kernel&m=136845723902257&w=2
>    This bug can only be triggered by root but it was still a real
>    bug where "int" was used instead of "unsigned int".
> 
> If I see that the type is cast using "min_t(u8," then I will assume
> that "XXX" is a u8.
> 
> It is also possible that the author chose "min_t(u8," when neither
> side of the comparison is a u8.  The common reason for that is
> because they are assigning the result to an u8 variable.  But from
> the immediate context I would see that that is not the case here.
> 

After read the bottom half of your that mail, we really need not talk
the details again.

> But again, in your specific cast casting it to an int is fine.  It
> doesn't cause a bug.  It only takes a little extra time for a
> reviewer.
> 

I will send min_t(u8..., in our case, it is better than min_t(int....


> Whatever...  It's up to you.  Send any patch which applies and has
> the proper signed-off-by and I will ack it.  :P
> 

Oh, it is my fault, I provide the incorrect signed-off-by, I make this
patch in another machine which forgot to add the default signed-off-by.
I should notice next time.


And it seems the patch sent too late, the original patch has already
applied into next tree, so I will send the fix up patch for it.


Thanks.
-- 
Chen Gang

Asianux Corporation
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux