| Message ID | 1283075566-27441-2-git-send-email-julia@diku.dk (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 1f7aac6eb585f92756603341cb1d770c797c4867 |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
Julia Lawall schrieb: > Add a call to of_node_put in the error handling code following a call to > of_find_node_by_path. > > The semantic match that finds this problem is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @r exists@ > local idexpression x; > expression E,E1; > statement S; > @@ > > *x = > (of_find_node_by_path > |of_find_node_by_name > |of_find_node_by_phandle > |of_get_parent > |of_get_next_parent > |of_get_next_child > |of_find_compatible_node > |of_match_node > )(...); > ... > if (x == NULL) S > <... when != x = E > *if (...) { > ... when != of_node_put(x) > when != if (...) { ... of_node_put(x); ... } > ( > return <+...x...+>; > | > * return ...; > ) > } > ...> > of_node_put(x); > // </smpl> > > Signed-off-by: Julia Lawall <julia@diku.dk> > > --- > drivers/macintosh/via-pmu-led.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c > index d242976..19c3718 100644 > --- a/drivers/macintosh/via-pmu-led.c > +++ b/drivers/macintosh/via-pmu-led.c > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void) > if (dt == NULL) > return -ENODEV; > model = of_get_property(dt, "model", NULL); > - if (model == NULL) > + if (model == NULL) { > + of_node_put(dt); > return -ENODEV; > + } > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && > strncmp(model, "iBook", strlen("iBook")) != 0 && > strcmp(model, "PowerMac7,2") != 0 && > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here (what is done in the last cmp). re, wh
On Tue, 31 Aug 2010, walter harms wrote: > > > Julia Lawall schrieb: > > Add a call to of_node_put in the error handling code following a call to > > of_find_node_by_path. > > > > The semantic match that finds this problem is as follows: > > (http://coccinelle.lip6.fr/) > > > > // <smpl> > > @r exists@ > > local idexpression x; > > expression E,E1; > > statement S; > > @@ > > > > *x = > > (of_find_node_by_path > > |of_find_node_by_name > > |of_find_node_by_phandle > > |of_get_parent > > |of_get_next_parent > > |of_get_next_child > > |of_find_compatible_node > > |of_match_node > > )(...); > > ... > > if (x == NULL) S > > <... when != x = E > > *if (...) { > > ... when != of_node_put(x) > > when != if (...) { ... of_node_put(x); ... } > > ( > > return <+...x...+>; > > | > > * return ...; > > ) > > } > > ...> > > of_node_put(x); > > // </smpl> > > > > Signed-off-by: Julia Lawall <julia@diku.dk> > > > > --- > > drivers/macintosh/via-pmu-led.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c > > index d242976..19c3718 100644 > > --- a/drivers/macintosh/via-pmu-led.c > > +++ b/drivers/macintosh/via-pmu-led.c > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void) > > if (dt == NULL) > > return -ENODEV; > > model = of_get_property(dt, "model", NULL); > > - if (model == NULL) > > + if (model == NULL) { > > + of_node_put(dt); > > return -ENODEV; > > + } > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && > > strncmp(model, "iBook", strlen("iBook")) != 0 && > > strcmp(model, "PowerMac7,2") != 0 && > > > > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here > (what is done in the last cmp). Perhaps there are some characters after eg PowerBook that one doesn't want to compare with? julia
On Tue, Aug 31, 2010 at 06:08:19PM +0200, Julia Lawall wrote: > On Tue, 31 Aug 2010, walter harms wrote: > > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void) > > > if (dt == NULL) > > > return -ENODEV; > > > model = of_get_property(dt, "model", NULL); > > > - if (model == NULL) > > > + if (model == NULL) { > > > + of_node_put(dt); > > > return -ENODEV; > > > + } > > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && > > > strncmp(model, "iBook", strlen("iBook")) != 0 && > > > strcmp(model, "PowerMac7,2") != 0 && > > > > > > > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here > > (what is done in the last cmp). > > Perhaps there are some characters after eg PowerBook that one doesn't want > to compare with? Yes. The model property on powermacs always has the version number. strncmp makes sure that *all* PowerBooks and iBooks are matched. g.
On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote: > On Tue, 31 Aug 2010, walter harms wrote: > > > > > > > Julia Lawall schrieb: > > > Add a call to of_node_put in the error handling code following a call to > > > of_find_node_by_path. [...] > > > --- a/drivers/macintosh/via-pmu-led.c > > > +++ b/drivers/macintosh/via-pmu-led.c > > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void) > > > if (dt == NULL) > > > return -ENODEV; > > > model = of_get_property(dt, "model", NULL); > > > - if (model == NULL) > > > + if (model == NULL) { > > > + of_node_put(dt); > > > return -ENODEV; > > > + } > > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && > > > strncmp(model, "iBook", strlen("iBook")) != 0 && > > > strcmp(model, "PowerMac7,2") != 0 && > > > > > > > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here > > (what is done in the last cmp). > > Perhaps there are some characters after eg PowerBook that one doesn't want > to compare with? It seems to me that model has no '\0' in the end. If model is got from the hardware then we should double check it - maybe harware is buggy. Otherwise we'll overflow model. But why strcmp(model, "PowerMac7,2")? IMO it should be replaced with strncmp().
On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov <segooon@gmail.com> wrote: > On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote: >> On Tue, 31 Aug 2010, walter harms wrote: >> > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && >> > > strncmp(model, "iBook", strlen("iBook")) != 0 && >> > > strcmp(model, "PowerMac7,2") != 0 && >> > > >> > >> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here >> > (what is done in the last cmp). >> >> Perhaps there are some characters after eg PowerBook that one doesn't want >> to compare with? > > It seems to me that model has no '\0' in the end. If model is got from > the hardware then we should double check it - maybe harware is buggy. > Otherwise we'll overflow model. Model does have \0 at the end. This code is using strncmp to purposefully ignore the model suffix. > But why strcmp(model, "PowerMac7,2")? IMO it should be replaced > with strncmp(). We use strcmp when parsing the device tree because the the length of the model property string is unknown and in most cases we *must* match the exact entire string, such as with this PowerMac7,2 example. Using strncmp would also happen to match with something like "PowerMac7,2345" which is not the desired behaviour. g.
Grant Likely schrieb: > On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov <segooon@gmail.com> wrote: >> On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote: >>> On Tue, 31 Aug 2010, walter harms wrote: >>>>> if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && >>>>> strncmp(model, "iBook", strlen("iBook")) != 0 && >>>>> strcmp(model, "PowerMac7,2") != 0 && >>>>> >>>> is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here >>>> (what is done in the last cmp). >>> Perhaps there are some characters after eg PowerBook that one doesn't want >>> to compare with? >> It seems to me that model has no '\0' in the end. If model is got from >> the hardware then we should double check it - maybe harware is buggy. >> Otherwise we'll overflow model. > > Model does have \0 at the end. This code is using strncmp to > purposefully ignore the model suffix. > >> But why strcmp(model, "PowerMac7,2")? IMO it should be replaced >> with strncmp(). > > We use strcmp when parsing the device tree because the the length of > the model property string is unknown and in most cases we *must* match > the exact entire string, such as with this PowerMac7,2 example. Using > strncmp would also happen to match with something like > "PowerMac7,2345" which is not the desired behaviour. > hi Grant, whould you mind to use you explanation as comment in the code ? Tthat the strncpy/strcpy difference is important should be noted. that would be clearly a bonos with further audits. re, wh
diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c index d242976..19c3718 100644 --- a/drivers/macintosh/via-pmu-led.c +++ b/drivers/macintosh/via-pmu-led.c @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void) if (dt == NULL) return -ENODEV; model = of_get_property(dt, "model", NULL); - if (model == NULL) + if (model == NULL) { + of_node_put(dt); return -ENODEV; + } if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 && strncmp(model, "iBook", strlen("iBook")) != 0 && strcmp(model, "PowerMac7,2") != 0 &&
Add a call to of_node_put in the error handling code following a call to of_find_node_by_path. The semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // <smpl> @r exists@ local idexpression x; expression E,E1; statement S; @@ *x = (of_find_node_by_path |of_find_node_by_name |of_find_node_by_phandle |of_get_parent |of_get_next_parent |of_get_next_child |of_find_compatible_node |of_match_node )(...); ... if (x == NULL) S <... when != x = E *if (...) { ... when != of_node_put(x) when != if (...) { ... of_node_put(x); ... } ( return <+...x...+>; | * return ...; ) } ...> of_node_put(x); // </smpl> Signed-off-by: Julia Lawall <julia@diku.dk> --- drivers/macintosh/via-pmu-led.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)