Re: [RFC 01/15] scripts/dtc: fix most memory leaks in dtc

2013-10-03 Thread Fabien Parent 
Looping back the mailing-lists.

On Thu, Oct 3, 2013 at 4:23 PM, Fabien Parent fpar...@baylibre.com wrote:

 Hi David,


 On Wed, Oct 2, 2013 at 2:59 PM, David Gibson da...@gibson.dropbear.id.au 
 wrote:

 On Tue, Sep 24, 2013 at 06:52:07PM +0200, Benoit Cousson wrote:
  From: Fabien Parent fpar...@baylibre.com

 As noted elsewhere, please write patches against upstream dtc, not the
 version in the kernel.  git://git.kernel.org/pub/scm/utils/dtc/dtc.git

  There are a few memory leaks in dtc which until now were not that important
  since they were all in the parser and only one instance of the parser was 
  run
  per instance of dtc. The following commits will add a validation of dts 
  through
  schema which have the same syntax as dts, i.e. the parser of dts will be 
  reused
  to parse schema. The consequence is that instead of having the parser 
  running
  only one time for an instance of the dtc process, the parser will run
  many many times and thus the leak that were not important until now becomes
  urgent to fix.

 Hrm, yeah.  I'm aware that I haven't been very careful with memory
 leaks within the parser.  Essentially, I've been treating the process
 as a pool allocator with exactly one pool - I've even considered
 getting rid of those free()s we do have.

 If the parser's going to be invoked repeatedly, then, yes, that
 certainly needs fixing.  Whether individually fixing each leak or
 using a explicit pool allocator is a better option is less clear.


 I've chosen the path of using free()s since it was removing most (all?) 
 memory leaks from
 the parser and wasn't adding much more complexity to it. I don't think a pool 
 allocator would
 be useful to DTC in its current state, but it's true that with schemas which 
 are using
 the DTS syntax it could make a lot of sense to switch to a pool allocator.

 I guess I will wait until the discussion about this proposal has progressed 
 and see
 whether this patch should be rewritten using a pool allocator or not.


  dtc-lexer: Do not duplicate the string which contains literals because the
  string is directly converted afterward to an integer and is never used 
  again.
  livetree: Add a bunch of free helper functions to clean properly the
  dt.

 This is no good.  Making this assumption is a layering violation - if
 the parser was changed so that this literal wasn't converted until
 after another token was read, the yytext value copied in here would no
 longer be valid and would break horribly.


 Right, I've been lazy on this one and I took the easy road.



 
  Signed-off-by: Fabien Parent fpar...@baylibre.com
  Signed-off-by: Benoit Cousson bcous...@baylibre.com
  ---
   scripts/dtc/dtc-lexer.l |   2 +-
   scripts/dtc/dtc-lexer.lex.c_shipped |   2 +-
   scripts/dtc/dtc.c   |   1 +
   scripts/dtc/dtc.h   |   1 +
   scripts/dtc/livetree.c  | 108 
  +---
   5 files changed, 105 insertions(+), 9 deletions(-)
 
  diff --git a/scripts/dtc/dtc-lexer.l b/scripts/dtc/dtc-lexer.l
  index 3b41bfc..4f63fbf 100644
  --- a/scripts/dtc/dtc-lexer.l
  +++ b/scripts/dtc/dtc-lexer.l
  @@ -146,7 +146,7 @@ static int pop_input_file(void);
}
 
   V1([0-9]+|0[xX][0-9a-fA-F]+)(U|L|UL|LL|ULL)? {
  - yylval.literal = xstrdup(yytext);
  + yylval.literal = yytext;
DPRINT(Literal: '%s'\n, yylval.literal);
return DT_LITERAL;
}
  diff --git a/scripts/dtc/dtc-lexer.lex.c_shipped 
  b/scripts/dtc/dtc-lexer.lex.c_shipped
  index 2d30f41..5c0d27c 100644
  --- a/scripts/dtc/dtc-lexer.lex.c_shipped
  +++ b/scripts/dtc/dtc-lexer.lex.c_shipped
  @@ -1054,7 +1054,7 @@ case 10:
   YY_RULE_SETUP
   #line 148 dtc-lexer.l
   {
  - yylval.literal = xstrdup(yytext);
  + yylval.literal = yytext;
DPRINT(Literal: '%s'\n, yylval.literal);
return DT_LITERAL;
}
  diff --git a/scripts/dtc/dtc.c b/scripts/dtc/dtc.c
  index a375683..215ae92 100644
  --- a/scripts/dtc/dtc.c
  +++ b/scripts/dtc/dtc.c
  @@ -256,5 +256,6 @@ int main(int argc, char *argv[])
die(Unknown output format \%s\\n, outform);
}
 
  + free_dt(bi);
exit(0);
   }
  diff --git a/scripts/dtc/dtc.h b/scripts/dtc/dtc.h
  index 3e42a07..9c45fd2 100644
  --- a/scripts/dtc/dtc.h
  +++ b/scripts/dtc/dtc.h
  @@ -245,6 +245,7 @@ struct boot_info {
   struct boot_info *build_boot_info(struct reserve_info *reservelist,
  struct node *tree, uint32_t 
  boot_cpuid_phys);
   void sort_tree(struct boot_info *bi);
  +void free_dt(struct boot_info *bi);
 
   /* Checks */
 
  diff --git a/scripts/dtc/livetree.c b/scripts/dtc/livetree.c
  index b61465f..5c8692c 100644
  --- a/scripts/dtc/livetree.c
  +++ b/scripts/dtc/livetree.c
  @@ -20,6 +20,10 @@
 
   #include dtc.h

Re: [RFC 01/15] scripts/dtc: fix most memory leaks in dtc

2013-10-02 Thread David Gibson 
On Tue, Sep 24, 2013 at 06:52:07PM +0200, Benoit Cousson wrote:
 From: Fabien Parent fpar...@baylibre.com

As noted elsewhere, please write patches against upstream dtc, not the
version in the kernel.  git://git.kernel.org/pub/scm/utils/dtc/dtc.git

 There are a few memory leaks in dtc which until now were not that important
 since they were all in the parser and only one instance of the parser was run
 per instance of dtc. The following commits will add a validation of dts 
 through
 schema which have the same syntax as dts, i.e. the parser of dts will be 
 reused
 to parse schema. The consequence is that instead of having the parser running
 only one time for an instance of the dtc process, the parser will run
 many many times and thus the leak that were not important until now becomes
 urgent to fix.

Hrm, yeah.  I'm aware that I haven't been very careful with memory
leaks within the parser.  Essentially, I've been treating the process
as a pool allocator with exactly one pool - I've even considered
getting rid of those free()s we do have.

If the parser's going to be invoked repeatedly, then, yes, that
certainly needs fixing.  Whether individually fixing each leak or
using a explicit pool allocator is a better option is less clear.

 dtc-lexer: Do not duplicate the string which contains literals because the
 string is directly converted afterward to an integer and is never used again.
 livetree: Add a bunch of free helper functions to clean properly the
 dt.

This is no good.  Making this assumption is a layering violation - if
the parser was changed so that this literal wasn't converted until
after another token was read, the yytext value copied in here would no
longer be valid and would break horribly.

 
 Signed-off-by: Fabien Parent fpar...@baylibre.com
 Signed-off-by: Benoit Cousson bcous...@baylibre.com
 ---
  scripts/dtc/dtc-lexer.l |   2 +-
  scripts/dtc/dtc-lexer.lex.c_shipped |   2 +-
  scripts/dtc/dtc.c   |   1 +
  scripts/dtc/dtc.h   |   1 +
  scripts/dtc/livetree.c  | 108 
 +---
  5 files changed, 105 insertions(+), 9 deletions(-)
 
 diff --git a/scripts/dtc/dtc-lexer.l b/scripts/dtc/dtc-lexer.l
 index 3b41bfc..4f63fbf 100644
 --- a/scripts/dtc/dtc-lexer.l
 +++ b/scripts/dtc/dtc-lexer.l
 @@ -146,7 +146,7 @@ static int pop_input_file(void);
   }
  
  V1([0-9]+|0[xX][0-9a-fA-F]+)(U|L|UL|LL|ULL)? {
 - yylval.literal = xstrdup(yytext);
 + yylval.literal = yytext;
   DPRINT(Literal: '%s'\n, yylval.literal);
   return DT_LITERAL;
   }
 diff --git a/scripts/dtc/dtc-lexer.lex.c_shipped 
 b/scripts/dtc/dtc-lexer.lex.c_shipped
 index 2d30f41..5c0d27c 100644
 --- a/scripts/dtc/dtc-lexer.lex.c_shipped
 +++ b/scripts/dtc/dtc-lexer.lex.c_shipped
 @@ -1054,7 +1054,7 @@ case 10:
  YY_RULE_SETUP
  #line 148 dtc-lexer.l
  {
 - yylval.literal = xstrdup(yytext);
 + yylval.literal = yytext;
   DPRINT(Literal: '%s'\n, yylval.literal);
   return DT_LITERAL;
   }
 diff --git a/scripts/dtc/dtc.c b/scripts/dtc/dtc.c
 index a375683..215ae92 100644
 --- a/scripts/dtc/dtc.c
 +++ b/scripts/dtc/dtc.c
 @@ -256,5 +256,6 @@ int main(int argc, char *argv[])
   die(Unknown output format \%s\\n, outform);
   }
  
 + free_dt(bi);
   exit(0);
  }
 diff --git a/scripts/dtc/dtc.h b/scripts/dtc/dtc.h
 index 3e42a07..9c45fd2 100644
 --- a/scripts/dtc/dtc.h
 +++ b/scripts/dtc/dtc.h
 @@ -245,6 +245,7 @@ struct boot_info {
  struct boot_info *build_boot_info(struct reserve_info *reservelist,
 struct node *tree, uint32_t boot_cpuid_phys);
  void sort_tree(struct boot_info *bi);
 +void free_dt(struct boot_info *bi);
  
  /* Checks */
  
 diff --git a/scripts/dtc/livetree.c b/scripts/dtc/livetree.c
 index b61465f..5c8692c 100644
 --- a/scripts/dtc/livetree.c
 +++ b/scripts/dtc/livetree.c
 @@ -20,6 +20,10 @@
  
  #include dtc.h
  
 +static void free_node_list(struct node *n);
 +static void free_node(struct node *n);
 +static void free_property(struct property *p);
 +
  /*
   * Tree building functions
   */
 @@ -144,7 +148,7 @@ struct node *merge_nodes(struct node *old_node, struct 
 node *new_node)
  
   /* Add new node labels to old node */
   for_each_label_withdel(new_node-labels, l)
 - add_label(old_node-labels, l-label);
 + add_label(old_node-labels, xstrdup(l-label));
  
   /* Move properties from the new node to the old node.  If there
* is a collision, replace the old value with the new */
 @@ -156,7 +160,7 @@ struct node *merge_nodes(struct node *old_node, struct 
 node *new_node)
  
   if (new_prop-deleted) {
   delete_property_by_name(old_node, new_prop-name);
 - free(new_prop);
 +

[RFC 01/15] scripts/dtc: fix most memory leaks in dtc

2013-09-24 Thread Benoit Cousson 
From: Fabien Parent fpar...@baylibre.com

There are a few memory leaks in dtc which until now were not that important
since they were all in the parser and only one instance of the parser was run
per instance of dtc. The following commits will add a validation of dts through
schema which have the same syntax as dts, i.e. the parser of dts will be reused
to parse schema. The consequence is that instead of having the parser running
only one time for an instance of the dtc process, the parser will run
many many times and thus the leak that were not important until now becomes
urgent to fix.

dtc-lexer: Do not duplicate the string which contains literals because the
string is directly converted afterward to an integer and is never used again.
livetree: Add a bunch of free helper functions to clean properly the dt.

Signed-off-by: Fabien Parent fpar...@baylibre.com
Signed-off-by: Benoit Cousson bcous...@baylibre.com
---
 scripts/dtc/dtc-lexer.l |   2 +-
 scripts/dtc/dtc-lexer.lex.c_shipped |   2 +-
 scripts/dtc/dtc.c   |   1 +
 scripts/dtc/dtc.h   |   1 +
 scripts/dtc/livetree.c  | 108 +---
 5 files changed, 105 insertions(+), 9 deletions(-)

diff --git a/scripts/dtc/dtc-lexer.l b/scripts/dtc/dtc-lexer.l
index 3b41bfc..4f63fbf 100644
--- a/scripts/dtc/dtc-lexer.l
+++ b/scripts/dtc/dtc-lexer.l
@@ -146,7 +146,7 @@ static int pop_input_file(void);
}
 
 V1([0-9]+|0[xX][0-9a-fA-F]+)(U|L|UL|LL|ULL)? {
-   yylval.literal = xstrdup(yytext);
+   yylval.literal = yytext;
DPRINT(Literal: '%s'\n, yylval.literal);
return DT_LITERAL;
}
diff --git a/scripts/dtc/dtc-lexer.lex.c_shipped 
b/scripts/dtc/dtc-lexer.lex.c_shipped
index 2d30f41..5c0d27c 100644
--- a/scripts/dtc/dtc-lexer.lex.c_shipped
+++ b/scripts/dtc/dtc-lexer.lex.c_shipped
@@ -1054,7 +1054,7 @@ case 10:
 YY_RULE_SETUP
 #line 148 dtc-lexer.l
 {
-   yylval.literal = xstrdup(yytext);
+   yylval.literal = yytext;
DPRINT(Literal: '%s'\n, yylval.literal);
return DT_LITERAL;
}
diff --git a/scripts/dtc/dtc.c b/scripts/dtc/dtc.c
index a375683..215ae92 100644
--- a/scripts/dtc/dtc.c
+++ b/scripts/dtc/dtc.c
@@ -256,5 +256,6 @@ int main(int argc, char *argv[])
die(Unknown output format \%s\\n, outform);
}
 
+   free_dt(bi);
exit(0);
 }
diff --git a/scripts/dtc/dtc.h b/scripts/dtc/dtc.h
index 3e42a07..9c45fd2 100644
--- a/scripts/dtc/dtc.h
+++ b/scripts/dtc/dtc.h
@@ -245,6 +245,7 @@ struct boot_info {
 struct boot_info *build_boot_info(struct reserve_info *reservelist,
  struct node *tree, uint32_t boot_cpuid_phys);
 void sort_tree(struct boot_info *bi);
+void free_dt(struct boot_info *bi);
 
 /* Checks */
 
diff --git a/scripts/dtc/livetree.c b/scripts/dtc/livetree.c
index b61465f..5c8692c 100644
--- a/scripts/dtc/livetree.c
+++ b/scripts/dtc/livetree.c
@@ -20,6 +20,10 @@
 
 #include dtc.h
 
+static void free_node_list(struct node *n);
+static void free_node(struct node *n);
+static void free_property(struct property *p);
+
 /*
  * Tree building functions
  */
@@ -144,7 +148,7 @@ struct node *merge_nodes(struct node *old_node, struct node 
*new_node)
 
/* Add new node labels to old node */
for_each_label_withdel(new_node-labels, l)
-   add_label(old_node-labels, l-label);
+   add_label(old_node-labels, xstrdup(l-label));
 
/* Move properties from the new node to the old node.  If there
 * is a collision, replace the old value with the new */
@@ -156,7 +160,7 @@ struct node *merge_nodes(struct node *old_node, struct node 
*new_node)
 
if (new_prop-deleted) {
delete_property_by_name(old_node, new_prop-name);
-   free(new_prop);
+   free_property(new_prop);
continue;
}
 
@@ -165,7 +169,7 @@ struct node *merge_nodes(struct node *old_node, struct node 
*new_node)
if (streq(old_prop-name, new_prop-name)) {
/* Add new labels to old property */
for_each_label_withdel(new_prop-labels, l)
-   add_label(old_prop-labels, l-label);
+   add_label(old_prop-labels, 
xstrdup(l-label));
 
old_prop-val = new_prop-val;
old_prop-deleted = 0;
@@ -191,7 +195,7 @@ struct node *merge_nodes(struct node *old_node, struct node 
*new_node)
 
if (new_child-deleted) {
delete_node_by_name(old_node, new_child-name);
-   free(new_child);
+   free_node(new_child);