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

[Shaobo He <shaobo@cs.utah.edu>]

Hi Laurent,

Thank you very much for your reply. While I was working on a patch, I realized 
that if we drop the two problematic NULL checks, we essentially eliminate one 
path of `v4l2_m2m_get_vq` where a NULL pointer is returned. So here comes a 
question, we may want to remove all the NULL checks of the return values of this 
function, for example in line 574 of media/platform/coda/coda-common.c. The 
reason is that if we conclude that `get_queue_ctx` cannot return a NULL pointer, 
then &q_ctx->q cannot be a NULL pointer as well because it points to the same 
address as `q_ctx` does.

If we agree on the NULL checks of the return values of `v4l2_m2m_get_vq` being 
unnecessary, I'll remove them in the patch.

Best,
Shaobo
On 2019/2/21 14:50, Laurent Pinchart wrote:
> 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 ? :-)
>