Re: [RFC][PATCH] media: v4l2-ctrls: add more NULL pointer checks

[Hans Verkuil]

Hi Sergey,

I recommend that you wait a bit until these two patches are merged:

https://patchwork.linuxtv.org/patch/61897/
https://patchwork.linuxtv.org/patch/61898/

I'm about to post a PR for these (and others), so hopefully these will
get merged soon.

Regards,

	Hans

On 16/04/2020 14:13, Hans Verkuil wrote:
> On 16/04/2020 13:32, Sergey Senozhatsky wrote:
>> On (20/04/16 10:53), Hans Verkuil wrote:
>> [..]
>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> @@ -2869,6 +2869,9 @@ EXPORT_SYMBOL(v4l2_ctrl_add_handler);
>>>>  
>>>>  bool v4l2_ctrl_radio_filter(const struct v4l2_ctrl *ctrl)
>>>>  {
>>>> +	if (WARN_ON(!ctrl))
>>>> +		return false;
>>>> +
>>>>  	if (V4L2_CTRL_ID2WHICH(ctrl->id) == V4L2_CTRL_CLASS_FM_TX)
>>>>  		return true;
>>>>  	if (V4L2_CTRL_ID2WHICH(ctrl->id) == V4L2_CTRL_CLASS_FM_RX)
>>>> @@ -3794,7 +3797,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl)
>>>>  	struct v4l2_ext_control c;
>>>>  
>>>>  	/* It's a driver bug if this happens. */
>>>> -	WARN_ON(!ctrl->is_int);
>>>> +	if (WARN_ON(!ctrl || !ctrl->is_int))
>>>> +		return -EINVAL;
>>>
>>> Just return 0 here. The return value is the control's value, not an error code.
>>> So all you can do here is return 0 in the absence of anything better.
>>
>> OK.
>>
>>>> +
>>>>  	c.value = 0;
>>>>  	get_ctrl(ctrl, &c);
>>>>  	return c.value;
>>>> @@ -4212,6 +4217,9 @@ EXPORT_SYMBOL(v4l2_s_ctrl);
>>>>  
>>>>  int __v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
>>>>  {
>>>> +	if (!ctrl)
>>>
>>> Change this to 'if (WARN_ON(!ctrl))'
>>>
>>> I don't think NULL pointers should be silently ignored: it really
>>> indicates a driver bug. It it certainly a good idea to WARN instead.
>>
>> Should WARN_ON() be only in unlocked versions of ctrl API? It probably
>> would make sense to add WARNs to both - e.g. to v4l2_ctrl_s_ctrl() and
> 
> Yes, it should be done for both.
> 
>> to __v4l2_ctrl_s_ctrl(). By the way, why don't locked and unlocked
>> versions live together in v4l2-ctrls.c file? Any reason for, e.g.,
>> v4l2_ctrl_s_ctrl() to be in header and __v4l2_ctrl_s_ctrl() to be C-file?
> 
> The v4l2_ctrl_s_ctrl() work fine as a static inline (only compiled if
> they are actually used). But with an additional 'if (WARN_ON(!ctrl))'
> it becomes a bit questionable. I would not be opposed if these static
> inlines are now moved into the source code.
> 
> Regards,
> 
> 	Hans
> 
>>
>>> The same is true for the functions below.
>>
>> OK.
>>
>> 	-ss
>>
>