netdev - Re: [patch -next v2] mv643xx_eth: potential null dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 25 Jul 2010 17:54:12 +0200
From:	Lennert Buytenhek <buytenh@...tstofly.org>
To:	walter harms <wharms@....de>
Cc:	Dan Carpenter <error27@...il.com>, Joe Perches <joe@...ches.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jiri Pirko <jpirko@...hat.com>,
	Denis Kirjanov <kirjanov@...il.com>,
	Saeed Bishara <saeed@...vell.com>, netdev@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [patch -next v2] mv643xx_eth: potential null dereference

On Sun, Jul 25, 2010 at 04:47:17PM +0200, walter harms wrote:

> >> IMHO it would be better to make sure that pd->t_clk,pd->tx_csum_limit
> >> have usefull values than adding a check but this is up to the maintainer.
> > 
> > I don't see the point of that at all.  We check against zero to see
> > whether the caller bothered to fill in the field at all, but if the
> > caller wants to pass in bogus values, that's up to the caller.
> 
> at first i have to admit i looked only at the patch.
> for me the situation looks this way:
> 
> You check the values for 0 (and uses default) or take what ever in pd is.
> The current code is setup like:
> 
>   1. check if pd is set
>   2. check if pd->value is non zero and use it
> 
> the whole "check X" can be avoided if you could create a pd with all values
> set to default and just take what comes from the user.

Why would you want to avoid "check X"?


> my objection agains this kind of code is that it is not obvious
> what some one is trying to archive
> (pd != NULL && pd->t_clk != 0) ? pd->t_clk : 133000000;
> 
> the pd check means: do i have a configuration  ?

Yep.


> the pd->t_clk != 0 means: should i use then or default ?

Yep.


> This is mixing two very different questions.

And?

You're arguing against the use of 'if (a && b)' in general?


> therefore my idea in the last posting to have a default init if (!pd)
> and then use the else to make clear that additional checks for
> pd->value are expected.

Again, I don't see the point of this, and I think we're just splitting
hairs here and wasting everyone's time.


> this this is the init code readability and simplicity should be king.

'if (foo != NULL && foo->value <op> <value>)' is a fairly common C
construct, in my opinion, and I don't see how expanding it into a
multi-line nested if construct will make it either more readable or
simpler.


> hope that helps,

Not really.


thanks,
Lennert
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html