[U-Boot] [PATCH 1/2] net/eth.c: make eth_get_dev_by_name(NULL) safe

[Detlev Zundel]

Hi Mike,

> On Monday, July 11, 2011 05:53:49 Helmut Raiger wrote:
>> On 07/07/2011 07:46 PM, Mike Frysinger wrote:
>> > those NULL checks should not be necessary either.  a correctly written
>> > networking driver should only register itself with the miiphy layer
>> > when it has successfully registered itself with the eth layer.  thus
>> > any of the miiphy callbacks should always come in with a name that is
>> > found via eth_get_dev_by_name().
>> 
>> You are right, in a perfect world nobody ever falters.
>
> you missed the point.  either the driver works, or it doesnt.  this func is 
> used in such a way that the behavior is fairly deterministic (as i clearly 
> laid out).  it isnt relying on allocated memory that could fall to be 
> allocated for example.

I for myself am also thinking of code that will be written in the
future, i.e. probably new uses of eth_get_dev_by_name.  Saving time in
this development because errors are caught early is a good thing IMHO.

>> > checking for NULLs here and gracefully returning is unnecessary
>> > overhead imo as you're only catering to broken code.  fix the broken
>> > drivers instead.
>> 
>> I could not find drivers that have the potential of delivering NULLs to
>> the miiphy_read and write functions, I simply refused to test at  this
>> level. Either there is a potential of having NULL passed then the test
>> should be in eth_get_dev_by_name() or there is none then we should
>> simply leave it away.
>
> i did go through the level of detail and showed the call graphs ... none of 
> which should allow a driver tested as working to even once hit the NULL path.

As I said, these are tha call graphs currently existing...

>> I'm rather indifferent to either solution.
>> And about 'catering to broken code': It must be broken in 2 ways, first
>> it must pass a NULL to the function and second it must ignore the return
>> code.
>
> not necessarily.  many platforms will abort on NULL pointer accesses.
>
>> > by your logic, why put the NULL check in eth_get_dev_by_name() ?  why
>> > not handle strcmp(NULL, NULL) too ?  then eth_get_dev_by_name() would
>> > automatically get "fixed" as would all other call points.
>> 
>> For strcmp() you have several issues at hand: Compatibility, performance
>> and even a logical problem. What should be the result in case one of the
>> arguments is NULL (or both).
>
> fair enough on the API, but your performance aspect is kind of hard to swallow 
> when you turn around and say that NULL pointer checking elsewhere is 
> minuscule.
>
>> And finally we are discussing a few _chararacters_ of code and a
>> probably a single assembly instruction.
>
> it's not a single assembly insn.  try generating it.  it adds like 8 to my 
> platform, mostly because of the increased register pressure.

On PowerPC with ELDK 4.2 this is the difference:

before:

00000004 g     F .text.eth_get_dev_by_name      00000080 eth_get_dev_by_name

after:

00000004 g     F .text.eth_get_dev_by_name      00000084 eth_get_dev_by_name


So at least for this arch it is indeed one word difference.

> but the point isnt the impact of this single check.  it sets the
> precedence that every function in u-boot that takes a pointer should
> start over protecting itself against poorly written code originating
> elsewhere.  now your "few characters" is quite a bit more.

I still stand by what I said that if we have functions that can be
called from many places (i.e. "library"-like), then the functions should
be conservative in what they expect.  Tightly coupled code can be looser
in this respect.  Maybe our disagreement stems from the fact that you
consider this function to be "tightly coupled" and not really library
like?

> what i wouldnt mind is annotating the prototype with gcc attributes saying 
> that the argument is nonnull.
> ...
> #define __nonnull(x) __attribute__((__nonnull__ x))
> ...
> extern struct eth_device *eth_get_dev_by_name(const char *devname) 
> __nonnull(1);
> ...

This can only catch calls the compiler can statically derive, but still
I think it is a good thing.

Cheers
  Detlev

-- 
Don't trust everything you read, and don't assume every poster in
a thread is actually relevant to the problem.
        -- Stefan Monnier <jwvlj1gk44h.fsf-monnier+emacs at gnu.org>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de