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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Looping back the mailing-lists.

On Thu, Oct 3, 2013 at 4:23 PM, Fabien Parent <fparent@xxxxxxxxxxxx> wrote:
>
> Hi David,
>
>
> On Wed, Oct 2, 2013 at 2:59 PM, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> On Tue, Sep 24, 2013 at 06:52:07PM +0200, Benoit Cousson wrote:
>> > From: Fabien Parent <fparent@xxxxxxxxxxxx>
>>
>> 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 <fparent@xxxxxxxxxxxx>
>> > Signed-off-by: Benoit Cousson <bcousson@xxxxxxxxxxxx>
>> > ---
>> >  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);
>> >                       continue;
>> >               }
>> >
>> > @@ -211,7 +215,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>> >
>> >       /* The new node contents are now merged into the old node.  Free
>> >        * the new node. */
>> > -     free(new_node);
>> > +     free_node(new_node);
>> >
>> >       return old_node;
>> >  }
>> > @@ -532,13 +536,13 @@ cell_t get_node_phandle(struct node *root, struct node *node)
>> >       if (!get_property(node, "linux,phandle")
>> >           && (phandle_format & PHANDLE_LEGACY))
>> >               add_property(node,
>> > -                          build_property("linux,phandle",
>> > +                          build_property(xstrdup("linux,phandle"),
>> >                                           data_append_cell(empty_data, phandle)));
>> >
>> >       if (!get_property(node, "phandle")
>> >           && (phandle_format & PHANDLE_EPAPR))
>> >               add_property(node,
>> > -                          build_property("phandle",
>> > +                          build_property(xstrdup("phandle"),
>> >                                           data_append_cell(empty_data, phandle)));
>> >
>> >       /* If the node *does* have a phandle property, we must
>> > @@ -707,3 +711,93 @@ void sort_tree(struct boot_info *bi)
>> >       sort_reserve_entries(bi);
>> >       sort_node(bi->dt);
>> >  }
>> > +
>> > +static void free_marker_list(struct marker *m)
>> > +{
>> > +     struct marker *marker, *marker_next;
>> > +
>> > +     if (!m)
>> > +             return;
>> > +
>> > +     for (marker = m, marker_next = marker ? marker->next : NULL;
>> > +          marker;
>> > +          marker = marker_next, marker_next = marker ? marker->next : NULL) {
>>
>> Rather hard to read version of safe-against-free list iteration.
>>
>
> Agreed.
>
>>
>> > +             free(marker->ref);
>> > +             free(marker);
>> > +     }
>> > +}
>> > +
>> > +static void free_label_list(struct label *l)
>> > +{
>> > +     struct label *label, *label_next;
>> > +
>> > +     if (!l)
>> > +             return;
>> > +
>> > +     for (label = l, label_next = label ? label->next : NULL;
>> > +          label;
>> > +          label = label_next, label_next = label ? label->next : NULL) {
>> > +             free(label->label);
>> > +             free(label);
>> > +     }
>> > +}
>> > +
>> > +static void free_property(struct property *p)
>> > +{
>> > +     if (!p)
>> > +             return;
>> > +
>> > +     free_label_list(p->labels);
>> > +     free_marker_list(p->val.markers);
>> > +     free(p->val.val);
>> > +     free(p->name);
>> > +     free(p);
>> > +}
>> > +
>> > +static void free_property_list(struct property *p)
>> > +{
>> > +     struct property *next;
>> > +
>> > +     if (!p)
>> > +             return;
>> > +
>> > +     for (next = p->next; p; p = next, next = p ? p->next : NULL)
>> > +             free_property(p);
>> > +}
>> > +
>> > +static void free_node(struct node *n)
>> > +{
>> > +     if (!n)
>> > +             return;
>> > +
>> > +     free_node_list(n->children);
>> > +     free_label_list(n->labels);
>> > +     free_property_list(n->proplist);
>> > +     free(n->fullpath);
>> > +     if (n->name && *n->name)
>>
>> *n->name .. so, the name can (and must) be statically allocated if
>> it's exactly "".  Not a useful optimization, I think.
>
>
> True.
>
>>
>>
>> > +             free(n->name);
>> > +     free(n);
>> > +}
>> > +
>> > +static void free_node_list(struct node *n)
>> > +{
>> > +     struct node *next;
>> > +
>> > +     if (!n)
>> > +             return;
>> > +
>> > +     for (next = n->next_sibling;
>> > +          n;
>> > +          n = next, next = n ? n->next_sibling : NULL) {
>> > +             free_node(n);
>> > +     }
>> > +}
>> > +
>> > +void free_dt(struct boot_info *bi)
>> > +{
>> > +     if (!bi)
>> > +             return;
>> > +
>> > +     free_node_list(bi->dt);
>> > +     free(bi);
>> > +}
>>
>> --
>> David Gibson                    | I'll have my music baroque, and my code
>> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>>                                 | _way_ _around_!
>> http://www.ozlabs.org/~dgibson
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux