Re: [PATCH] Remove deductively redundant NULL pointer checks
From: Ezequiel Garcia <hidden>
Date: 2019-03-03 23:34:30
Also in:
linux-arm-kernel, linux-media, linux-renesas-soc, linux-rockchip, lkml
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 <redacted>
Hi Shaobo, It seems this is v2 of 1551128631-19713-1-git-send-email-shaobo@cs.utah.edu, 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...
quoted hunk ↗ jump to hunk
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 capturediff --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