Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-09-01 Thread walter harms 


Grant Likely schrieb:
 On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov sego...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-08-31 Thread Julia Lawall 
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 ju...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-08-31 Thread Grant Likely 
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.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-08-31 Thread Vasiliy Kulikov 
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().

-- 
Vasiliy
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-08-31 Thread Grant Likely 
On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov sego...@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.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-08-31 Thread walter harms 


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 ju...@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



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

2010-08-29 Thread Julia Lawall 
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 ju...@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 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev