Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash

On Tue, Feb 02, 2021 at 03:25:49PM -0600, FUZZ DR wrote:

Sorry about the missing description, I have a description at my local
commit. But the commit description disappeared when I used git send-email
to submit this patch.

backlight: pcf50633: pdata may be a null pointer, null pointer dereference
causes crash
pdata has been checked  at line 120 before dereference. However, it is used
without check at line 130. So just add the check,

To be clear what your analyzer is reporting is a mismatch of programmer
intent.

In line 120 suggests a belief that pdata could be NULL whilst line 130
suggests a belief that pdata is never NULL. Your analyzer cannot not tell
you which belief is incorrect, only that there is a mismatch.

It is better to write a default value to the register if the ramp_time has
a default value. Then it does not need to return -EINVAL. It will keep
consistent with the behavior at line 120.

I disagree.

You have assumed that line 120 is correct and that this code needs to
support the case where pdata is NULL. However eleven years of history
disprove this! This code is never called without valid platform data.

Therefore we should fix the assumption on line 120 by making it clear
that this code code expects non-NULL platform data. Given there are
developers working to eliminate this kind of platform data structure
(by adopting device properties instead) I don't want to make their
life harder by implementing unused and untested special cases that
they would have to reason about.


Daniel.

Daniel Thompson [off-list ref] 于2021年2月2日周二 上午3:25写道：

quoted

On Mon, Feb 01, 2021 at 08:41:38AM -0600, Wenjia Zhao wrote:

quoted

Signed-off-by: Wenjia Zhao <redacted>

There should be a patch description here explaining why the patch
is needed and how it works.

quoted

---
 drivers/video/backlight/pcf50633-backlight.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/pcf50633-backlight.c

b/drivers/video/backlight/pcf50633-backlight.c

quoted

index 540dd338..43267af 100644

--- a/drivers/video/backlight/pcf50633-backlight.c
+++ b/drivers/video/backlight/pcf50633-backlight.c

@@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device

*pdev)

quoted

      platform_set_drvdata(pdev, pcf_bl);

-     pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM,

pdata->ramp_time);

quoted

+  if (pdata)
+    pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM,

pdata->ramp_time);

Assuming you found this issue using a static analyzer then I think it
might be better to if an "if (!pdata) return -EINVAL" further up the
file instead.

In other words it is better to "document" (via the return code) that the
code does not support pdata == NULL than to add another untested code
path.


Daniel.