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 ? :-)
