lkml.org 
[lkml]   [2019]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] media: v4l2-core: Shifting signed 32-bit value by 31 bits error
From
Date
On 6/6/19 12:33 AM, Hans Verkuil wrote:
> On 6/6/19 5:22 AM, Randy Dunlap wrote:
>> On 6/5/19 2:53 PM, Shuah Khan wrote:
>>> Fix the following cppcheck error:
>>>
>>> Checking drivers/media/v4l2-core/v4l2-ioctl.c ...
>>> [drivers/media/v4l2-core/v4l2-ioctl.c:1370]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour
>>>
>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 6859bdac86fe..333e387bafeb 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1364,7 +1364,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>> (char)((fmt->pixelformat >> 8) & 0x7f),
>>> (char)((fmt->pixelformat >> 16) & 0x7f),
>>> (char)((fmt->pixelformat >> 24) & 0x7f),
>>> - (fmt->pixelformat & (1 << 31)) ? "-BE" : "");
>>> + (fmt->pixelformat & BIT(31)) ? "-BE" : "");
>>> break;
>>> }
>>> }
>>>
>>
>> If this builds, I guess #define BIT(x) got pulled in indirectly
>> since bits.h nor bitops.h is currently #included in that source file.
>>

It does build. You are right that I should have included bitops.h

>> Documentation/process/submit-checklist.rst rule #1 says:
>> 1) If you use a facility then #include the file that defines/declares
>> that facility. Don't depend on other header files pulling in ones
>> that you use.
>>
>> Please add #include <linux/bits or bitops.h>
>>
>
> I'm not sure about this patch. '1 << 31' is used all over in the kernel,
> including in public headers (e.g. media.h, videodev2.h).
>
> It seems arbitrary to change it only here, but not anywhere else.
>

Right. We have several places in the kernel that do that.

> In this particular example for the fourcc handling I would prefer to just
> use '1U << 31', both in v4l2-ioctl.c and videodev2.h.
>

If you would like to take the patch, I can send v2 fixing it using
1U << 31 - This is simpler since it doesn't nee additional includes.

> A separate patch doing the same for MEDIA_ENT_ID_FLAG_NEXT in media.h would
> probably be a good idea either: that way the public API at least will do
> the right thing.
>

I should have explained it better. I wanted to start with one or two
places first to see if it is worth our time to fix these:

The full kernel cppcheck log for "Shifting signed 32-bit value by 31
bits is undefined behaviour" can be found at:

https://drive.google.com/file/d/19Xu7UqBGJ7BpzxEp92ZQYb6F8UPrk3z3/view

thanks,
-- Shuah

\
 
 \ /
  Last update: 2019-06-11 21:42    [W:0.085 / U:0.244 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site