Re: [PATCH] Remove deductively redundant NULL pointer checks

From: Ezequiel Garcia
Date: Sun Mar 03 2019 - 18:34:37 EST


On Tue, 2019-02-26 at 17:43 -0700, Shaobo He wrote:
> The fixes included in this commit essentially removes NULL pointer
> checks on the return values of function `get_queue_ctx` as well as
> `v4l2_m2m_get_vq` defined in file v4l2-mem2mem.c.
>
> Function `get_queue_ctx` is very unlikely to return a NULL pointer
> because its return value is an address composed of the base address
> pointed by `m2m_ctx` and an offset of field `out_q_ctx` or `cap_q_ctx`.
> Since the offset of either field is not 0, for the return value to be
> NULL, pointer `m2m_ctx` must be a very large unsigned value such that
> its addition to the offset overflows to NULL which may be undefined
> according to this post:
> https://wdtz.org/catching-pointer-overflow-bugs.html. Moreover, even if
> `m2m_ctx` is NULL, the return value cannot be NULL, either. Therefore, I
> think it is reasonable to conclude that the return value of function
> `get_queue_ctx` cannot be NULL.
>
> Given the return values of `get_queue_ctx` not being NULL, we can follow
> a similar reasoning to conclude that the return value of
> `v4l2_mem_get_vq` cannot be NULL since its return value is the same
> address as the return value of `get_queue_ctx`. Therefore, this patch
> also removes NULL pointer checks on the return values of
> `v4l2_mem_get_vq`.
>
> Signed-off-by: Shaobo He <shaobo@xxxxxxxxxxx>

Hi Shaobo,

It seems this is v2 of 1551128631-19713-1-git-send-email-shaobo@xxxxxxxxxxx,
and it should be marked as such.

Do you think you can read Documentation/process/submitting-patches.rst,
for your future patches?

Also, two comments...

> drivers/media/platform/coda/coda-common.c | 4 ----
> drivers/media/platform/imx-pxp.c | 7 -------
> drivers/media/platform/m2m-deinterlace.c | 7 -------
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 7 -------
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 7 -------
> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 13 -------------
> drivers/media/platform/mx2_emmaprp.c | 7 -------
> drivers/media/platform/rcar_fdp1.c | 3 ---
> drivers/media/platform/rcar_jpu.c | 8 --------
> drivers/media/platform/rockchip/rga/rga.c | 4 ----
> drivers/media/platform/s5p-g2d/g2d.c | 4 ----
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 7 -------
> drivers/media/platform/sh_veu.c | 2 --
> drivers/media/platform/ti-vpe/vpe.c | 7 -------
> drivers/media/platform/vicodec/vicodec-core.c | 5 -----
> drivers/media/platform/vim2m.c | 7 -------
> drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ----
> 17 files changed, 103 deletions(-)
>
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 7518f01..ee1e05b 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -696,8 +696,6 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct v4l2_format *f,
> struct vb2_queue *vq;
>
> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (!vq)
> - return -EINVAL;
>
> q_data = get_q_data(ctx, f->type);
> if (!q_data)
> @@ -817,8 +815,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
> ctx->quantization = f->fmt.pix.quantization;
>
> dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> - if (!dst_vq)
> - return -EINVAL;
>
> /*
> * Setting the capture queue format is not possible while the capture
> diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
> index c1c2554..d079b3c 100644
> --- a/drivers/media/platform/imx-pxp.c
> +++ b/drivers/media/platform/imx-pxp.c
> @@ -1071,13 +1071,8 @@ static int pxp_enum_fmt_vid_out(struct file *file, void *priv,
>
> static int pxp_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
> {
> - struct vb2_queue *vq;
> struct pxp_q_data *q_data;
>
> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (!vq)
> - return -EINVAL;
> -

It seems your patch also removes unused code, but this is not really explained in the
commit log.

Perhaps it is better to split all these changes on their own patch:
one patch to remove dead code, and then another patch to remove unneeded null checks.

And also, I think you should add some comments, either in v4l2_m2m_get_vq's declaration
or definition, explaining that the return value cannot be NULL.

I have to say: I'm not a fan of "improvement" patches in code paths that
are anything but hot... but knock yourself out!

Thanks,
Eze