Re: [PATCH v1 2/9] pinctrl: tegra30: fix "Possible null pointer dereference: group" warning

[Lucas Stach <l.stach@pengutronix.de>]

Am Mittwoch, den 21.11.2018, 10:40 +0100 schrieb Oleksij Rempel:
> On Wed, Nov 21, 2018 at 10:16:58AM +0100, Lucas Stach wrote:
> > Am Dienstag, den 20.11.2018, 21:07 +0100 schrieb Oleksij Rempel:
> > > The code is correct but it takes more seconds for me to understand.
> > > And static code analyzer do not understand it at all.
> > > 
> > > > Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
> > > 
> > > ---
> > >  drivers/pinctrl/pinctrl-tegra30.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-tegra30.c b/drivers/pinctrl/pinctrl-tegra30.c
> > > index d9b49c57d..ffb04eebb 100644
> > > --- a/drivers/pinctrl/pinctrl-tegra30.c
> > > +++ b/drivers/pinctrl/pinctrl-tegra30.c
> > > @@ -658,8 +658,8 @@ static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl,
> > > > > > > >  			break;
> > > > > > > >  		}
> > > > > > > >  	}
> > > > > > > > -	/* if no matching drivegroup is found */
> > > > -	if (i == ctrl->drvdata->num_drvgrps)
> > > 
> > > +
> > > > > > > > +	if (!group)
> > > >  		return 0;
> > 
> > Huh? This is a pretty standard idiom in C codebases to check if we
> > broke out of a loop early.
> > 
> > Actually this change breaks the code, as this check is inside of an
> > outer loop that doesn't reinitialize the group variable. So while the
> > code as-is correctly checks if a group was found in the current
> > iteration of the outer loop, after this patch it also matches a group
> > that was found on a previous iteration of the outer loop.
> 
> Probably I still do not understand it:
> 
> static const struct pinctrl_tegra30_drvdata tegra124_drvdata = {
> 	.pingrps = tegra124_pin_groups,
> 	.num_pingrps = ARRAY_SIZE(tegra124_pin_groups),
> 	.drvgrps = tegra124_drive_groups,
> 	.num_drvgrps = ARRAY_SIZE(tegra124_drive_groups), 
> 	^^^^^ this is constant.
> };
> 
> static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl,
>                                         struct device_node *np)
> {
> 	const char *pins = NULL;
> 	const struct tegra_drive_pingroup *group = NULL;
> 	^^^^ here we init *group to NULL
> 
> 	int hsm = -1, schmitt = -1, pds = -1, pus = -1, srr = -1, srf = -1;
> 	int i;
> 	u32 __iomem *regaddr;
> 	u32 val;
> 
> 	if (of_property_read_string(np, "nvidia,pins", &pins))
> > 		return 0;
> 
> 	for (i = 0; i < ctrl->drvdata->num_drvgrps; i++) {
> 	^^^^ here we init i
> > 		if (!strcmp(pins, ctrl->drvdata->drvgrps[i].name)) {
> > 			group = &ctrl->drvdata->drvgrps[i];
> > 			^^^^^ -- only here group is not NULL
> > 			break;
> > 		}
> 	}
> 	/* if no matching drivegroup is found */
> 	if (i == ctrl->drvdata->num_drvgrps)
> 	^^^^^ if i == num_drvgrps, group is also NULL..
> > 		return 0;
> 
> I don't see any technical problems. Or i do oversee some thing?

Sorry, I've looked at the wrong function. This patch doesn't break
anything, but now makes the code inconsistent. In
pinctrl_tegra30_set_state() this idiom is needed to make the code
correct and I would rather have the groups searches consistent between
the 2 functions.

> > This is a prime example where static checker warnings can prompt wrong
> > fixes. Frankly codacy should smart up to correctly analyze the
> > controlflow interdependency.
> 
> Well, amount of real problems found by this code check is still higher.
> So why not to use it?

I'm not arguing against static checkers, but I'm saying that it can
sometimes be hard to get to the real issues, when the checker is
producing a significant amount of false positives. And I'm against
changing code just to make checkers happy, because every one of them
gets something different wrong...

But whatever, the code is still correct, so the patch can stay.

Regards,
Lucas



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox