Re: Problematic code in media/v4l2-core/v4l2-mem2mem.c

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Shaobo,

On Thu, Feb 21, 2019 at 11:11:32AM -0700, Shaobo wrote:
> Hello everyone,
> 
> I think I brought up this issue before but didn't resolve it completely. 
> Now I'd like to double check this and if we can agree on it, I'd also 
> like to submit a patch to fix it. The problem is that function 
> `get_queue_ctx` can never return a NULL pointer unless pointer overflow 
> occurs, which is very unlikely. To be more specific,
> 
> ```
> static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx 
> *m2m_ctx,
> 						enum v4l2_buf_type type)
> {
> 	if (V4L2_TYPE_IS_OUTPUT(type))
> 		return &m2m_ctx->out_q_ctx;
> 	else
> 		return &m2m_ctx->cap_q_ctx;
> }
> ```
> 
> The address returned by this function is either `(char*)m2m_ctx+968` or 
> `(char*)m2m_ctx+16`, so for it to be NULL, `m2m_ctx` must be a large 
> unsigned value. Yet the return value of this function is NULL-checked, 
> for example in v4l2_m2m_get_vq.
> 
> Please let me know if it makes sense.

It makes complete sense.

There are only two callers of get_queue_ctx() that check the return
value of the function. It may be argued that the intent was to check for
a NULL m2m_ctx, so you could replace those two checks with a NULL check
for m2m_ctx before calling get_queue_ctx(). However, given that nothing
is crashing, it may also be argued that the checks are unnecessary and
can be dropped completely. The best would be to review the call paths to
ensure the functions can indeed never be called with NULL, but a quick
look at the code shows no other NULL check in functions taking a m2m_ctx
pointer as argument, so I'd vote for just dropping the two offending
checks.

Care to submit a patch ? :-)

-- 
Regards,

Laurent Pinchart