| Message ID | 1309775392-8282-1-git-send-email-helmut.raiger@hale.at |
|---|---|
| State | Superseded |
| Headers | show |
On Monday, July 04, 2011 06:29:51 helmut.raiger@hale.at wrote: > eth_get_dev_by_name() is not safe to use for devname being NULL > as it uses strcmp. This patch makes it return NULL if devname NULL > is passed. i'm not sure about this. passing NULL is wrong, and the caller should catch that shouldnt it ? -mike
On 07/05/2011 05:44 AM, Mike Frysinger wrote: > On Monday, July 04, 2011 06:29:51 helmut.raiger@hale.at wrote: >> eth_get_dev_by_name() is not safe to use for devname being NULL >> as it uses strcmp. This patch makes it return NULL if devname NULL >> is passed. > i'm not sure about this. passing NULL is wrong, and the caller should catch > that shouldnt it ? > -mike So what is your suggestion how to deal with it? It returns: "There is no ethernet device with name NULL" This is pretty much the only thing it can return. The user of the function may handle this situation individually like: printf("ethernet device '%s' not found\n, devname); --> "ethernet device '(NULL)' not found". A panic on a NULL pointer de-reference is probably not helpful either. Helmut -- Scanned by MailScanner.
On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote: > On 07/05/2011 05:44 AM, Mike Frysinger wrote: > > On Monday, July 04, 2011 06:29:51 helmut.raiger@hale.at wrote: > >> eth_get_dev_by_name() is not safe to use for devname being NULL > >> as it uses strcmp. This patch makes it return NULL if devname NULL > >> is passed. > > > > i'm not sure about this. passing NULL is wrong, and the caller should > > catch that shouldnt it ? > > So what is your suggestion how to deal with it? in what situation is eth_get_dev_by_name(NULL) being called ? my suggestion would be to fix that call point since it's doing something wrong. -mike
On 07/06/2011 09:38 PM, Mike Frysinger wrote: > On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote: >> On 07/05/2011 05:44 AM, Mike Frysinger wrote: >>> On Monday, July 04, 2011 06:29:51 helmut.raiger@hale.at wrote: >>>> eth_get_dev_by_name() is not safe to use for devname being NULL >>>> as it uses strcmp. This patch makes it return NULL if devname NULL >>>> is passed. >>> i'm not sure about this. passing NULL is wrong, and the caller should >>> catch that shouldnt it ? >> So what is your suggestion how to deal with it? > in what situation is eth_get_dev_by_name(NULL) being called ? my suggestion > would be to fix that call point since it's doing something wrong. > -mike I couldn't find a situation where this might be the case. But as Luca Ceresoli pointed out in his e-mail, somewhere up the thread, that he tested for devname being NULL in his miiphy_read and write routines, I checked eth_get_dev_by_name() and found that it is vulnerable to passing a NULL pointer, hence the fix. Is there something missing for the patch to be acknowledged? It's hanging there quite a while now? Helmut -- Scanned by MailScanner.
Hi Helmut, > On 07/06/2011 09:38 PM, Mike Frysinger wrote: >> On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote: >>> On 07/05/2011 05:44 AM, Mike Frysinger wrote: >>>> On Monday, July 04, 2011 06:29:51 helmut.raiger@hale.at wrote: >>>>> eth_get_dev_by_name() is not safe to use for devname being NULL >>>>> as it uses strcmp. This patch makes it return NULL if devname NULL >>>>> is passed. >>>> i'm not sure about this. passing NULL is wrong, and the caller should >>>> catch that shouldnt it ? >>> So what is your suggestion how to deal with it? >> in what situation is eth_get_dev_by_name(NULL) being called ? my suggestion >> would be to fix that call point since it's doing something wrong. >> -mike > I couldn't find a situation where this might be the case. But as Luca > Ceresoli pointed out in his e-mail, somewhere up the thread, that he > tested for devname being NULL in his miiphy_read and write routines, I > checked eth_get_dev_by_name() and found that it is vulnerable to passing > a NULL pointer, hence the fix. > > Is there something missing for the patch to be acknowledged? > It's hanging there quite a while now? Actually I want to ack your patch. eth_get_dev_by_name is a project wide utility function and as such should be tolerant to being called with wrong parameters. Mike's argument of course also has merit to it, but because of the "library function" argument, I'd give it less priority. So Acked-by: Detlev Zundel <dzu@denx.de>
Hi Helmut,
Le 04/07/2011 12:29, helmut.raiger@hale.at a écrit :
> From: Helmut Raiger<helmut.raiger@hale.at>
Seems like your git send-email config does not have your name along with
your e-mail address, causing this From: to appear in the patch body (and
the mail itself to lack your name) -- git can handle this, I think, but
I'd be grateful if you fixed your config.
Amicalement,
On Thu, Jul 7, 2011 at 02:12, Helmut Raiger wrote: > On 07/06/2011 09:38 PM, Mike Frysinger wrote: >> On Wednesday, July 06, 2011 03:15:08 Helmut Raiger wrote: >>> On 07/05/2011 05:44 AM, Mike Frysinger wrote: >>>> On Monday, July 04, 2011 06:29:51 helmut.raiger@hale.at wrote: >>>>> eth_get_dev_by_name() is not safe to use for devname being NULL >>>>> as it uses strcmp. This patch makes it return NULL if devname NULL >>>>> is passed. >>>> >>>> i'm not sure about this. passing NULL is wrong, and the caller should >>>> catch that shouldnt it ? >>> >>> So what is your suggestion how to deal with it? >> >> in what situation is eth_get_dev_by_name(NULL) being called ? my >> suggestion >> would be to fix that call point since it's doing something wrong. > > I couldn't find a situation where this might be the case. But as Luca > Ceresoli pointed out in his e-mail, somewhere up the thread, that he tested > for devname being NULL in his miiphy_read and write routines, I checked > eth_get_dev_by_name() and found that it is vulnerable to passing a NULL > pointer, hence the fix. 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(). checking for NULLs here and gracefully returning is unnecessary overhead imo as you're only catering to broken code. fix the broken drivers instead. 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. -mike
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. > 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'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. > 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). The logic for eth_get_dev_by_name() is completely sane "There is no ethernet device named (NULL)", performance and compatibility don't matter. Your comparison is flawed. And finally we are discussing a few _chararacters_ of code and a probably a single assembly instruction. Helmut -- Scanned by MailScanner.
On 07/07/2011 06:46 PM, Albert ARIBAUD wrote: > Hi Helmut, > > Le 04/07/2011 12:29, helmut.raiger@hale.at a écrit : >> From: Helmut Raiger<helmut.raiger@hale.at> > > Seems like your git send-email config does not have your name along > with your e-mail address, causing this From: to appear in the patch > body (and the mail itself to lack your name) -- git can handle this, I > think, but I'd be grateful if you fixed your config. > > Amicalement, Hi Albert, I just checked my config, user and e-mail were fine, but I had the from configured in the sendemail section which obviously generates the 'From:' line. Do you like me to resend the patches? Helmut -- Scanned by MailScanner.
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. > > 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. > 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. 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. 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); ... -mike
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
On Tue, Jul 12, 2011 at 05:22, Detlev Zundel wrote: > Mike Frysinger wrote: >> 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? not really. i consider this to be "garbage-in garbage-out". imo, u-boot isnt a C library that should be padded with garbage checking all over. the result only helps broken systems (edge cases) while hindering the rest. i wouldnt have a problem with adopting an NDEBUG system, or perhaps adding assert()'s to this code. then people can easily opt-out of it all and for the people doing development, can easily turn things on. assert(name != NULL); the current miiphy system needs to be replaced (this runtime string based approach is crazy), but that's a completely different topic :). -mike
On 07/12/2011 11:22 AM, Detlev Zundel wrote: > > 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 the call graphs currently existing... This was also my trail. > > 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. > __nonnull__ is actually a optimization attribute, gcc removes tests for NULL in the function body, warnings are only generated if one literally writes: eth_get_dev_by_name(NULL), so 'statically derive' is already exageration. This really is no help at all. It would indeed establish a precendence to using an IMHO quite flawed attribute in gcc. If I had a vote, I'd be against it. The NDEBUG approach however, as Mike suggested, was what I was looking for in the first place. Helmut -- Scanned by MailScanner.
Hi Mike, [...] > not really. i consider this to be "garbage-in garbage-out". imo, > u-boot isnt a C library that should be padded with garbage checking > all over. the result only helps broken systems (edge cases) while > hindering the rest. > > i wouldnt have a problem with adopting an NDEBUG system, or perhaps > adding assert()'s to this code. then people can easily opt-out of it > all and for the people doing development, can easily turn things on. > assert(name != NULL); While developing, I certainly appreciate 'garbage-in error-out' and as your approach allows this, I believe we have reached a consensus. > the current miiphy system needs to be replaced (this runtime string > based approach is crazy), but that's a completely different topic :). I'm looking forward to your changes :) Thanks! Detlev
Hi Helmut, > On 07/12/2011 11:22 AM, Detlev Zundel wrote: > >> > 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 the call graphs currently existing... > > This was also my trail. > >> > 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. >> > > __nonnull__ is actually a optimization attribute, gcc removes > tests for NULL in the function body, warnings are only generated if > one literally writes: eth_get_dev_by_name(NULL), so 'statically > derive' > is already exageration. I just checked and can confirm that currently gcc does not do any static analysis of char* arguments - however in theory it could. > This really is no help at all. It would indeed establish a precendence > to using an IMHO quite flawed attribute in gcc. If I had a vote, I'd > be against it. I agree that how this is implemented in gcc is no big help. Rather than believing documentation I should have checked how this works before lobbying for it. > The NDEBUG approach however, as Mike suggested, was what I was > looking for in the first place. Great! Detlev
On 07/13/2011 01:46 PM, Detlev Zundel wrote: > >> The NDEBUG approach however, as Mike suggested, was what I was >> looking for in the first place. > Great! > Detlev > Again, not so great. U-boot uses all kinds of assert(), BUG_ON(), ASSERT() all over the place. This probably needs a project wide effort, which is why I simply threw in my NULL pointer check. Helmut -- Scanned by MailScanner.
Hi Helmut, Le 11/07/2011 12:10, Helmut Raiger a écrit : > On 07/07/2011 06:46 PM, Albert ARIBAUD wrote: >> Hi Helmut, >> >> Le 04/07/2011 12:29, helmut.raiger@hale.at a écrit : >>> From: Helmut Raiger<helmut.raiger@hale.at> >> >> Seems like your git send-email config does not have your name along >> with your e-mail address, causing this From: to appear in the patch >> body (and the mail itself to lack your name) -- git can handle this, I >> think, but I'd be grateful if you fixed your config. >> >> Amicalement, > Hi Albert, > > I just checked my config, user and e-mail were fine, but I had the from > configured in the sendemail section which obviously generates the > 'From:' line. Do you like me to resend the patches? Not needed for this patch (if that From: is ok with patchwork and Git) but if you send a V2 patch or for future patches, please do avoid the added From. Amicalement,
On Thursday, July 14, 2011 05:14:07 Helmut Raiger wrote: > On 07/13/2011 01:46 PM, Detlev Zundel wrote: > >> The NDEBUG approach however, as Mike suggested, was what I was > >> looking for in the first place. > > Again, not so great. U-boot uses all kinds of assert(), BUG_ON(), > ASSERT() all over the place. > This probably needs a project wide effort, which is why I simply threw > in my NULL pointer check. we tend to look at the long term picture with the best/correct solution rather than one-offs which get thrown away in the end -mike
On Wednesday, July 13, 2011 02:32:37 Helmut Raiger wrote: for future reference, please dont send html e-mails -mike
diff --git a/net/eth.c b/net/eth.c index 6523834..c75b7c9 100644 --- a/net/eth.c +++ b/net/eth.c @@ -107,7 +107,7 @@ struct eth_device *eth_get_dev_by_name(const char *devname) { struct eth_device *dev, *target_dev; - if (!eth_devices) + if (!eth_devices || !devname) return NULL; dev = eth_devices;