On Sun, Mar 06, 2016 at 04:23:08PM +0100, Martin Pieuchot wrote:
> On 28/02/16(Sun) 17:49, Patrick Wildt wrote:
> > Hi,
> >
> > If we're calling fdt_find_node() and do not actually find the node we're
> > looking for, we call strncmp with a NULL value.
> >
> > What happens is that we use fdt_child_node(node) to retrieve a child
> > and then use fdt_next_node(child) to go through the list of children.
> > If we do not find a child that matches the given name(s), it will
> > reach the end of the list. You know that you're at the end of the
> > list if the next token is not FDT_NODE_BEGIN. A child must start
> > with that token.
> >
> > Even though there's no child left, fdt_next_node(child) will not
> > return a NULL ptr, but instead return a pointer to the next token.
> > This means the for-loop will continue to run and call strncmp.
> > fdt_node_name(child) will return a NULL ptr, as the token behind
> > the ptr is not FDT_NODE_BEGIN.
>
> Is it possible to fix fdt_next_node() then?
That's a good question and is probably the better fix. fdt_next_node()
depends on recursion and expects itself to return a pointer to the
next node, no matter if it exists or not. With a bit of code
duplication this can be worked around. Basically by implementing
another function that did what fdt_next_node() did before, and
then have a check at the end of fdt_next_node() to only return
the node if it exists.
diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
index cdbecee..e6ec5d4 100644
--- sys/dev/ofw/fdt.c
+++ sys/dev/ofw/fdt.c
@@ -30,6 +30,7 @@ char *fdt_get_str(u_int32_t);
void *skip_property(u_int32_t *);
void *skip_props(u_int32_t *);
void *skip_node_name(u_int32_t *);
+void *fdt_skip_node(void *);
void *fdt_parent_node_recurse(void *, void *);
#ifdef DEBUG
void fdt_print_node_recurse(void *, int);
@@ -171,8 +172,36 @@ fdt_node_property(void *node, char *name, char **out)
}
/*
- * Retrieves next node, skipping all the children nodes of the pointed node
- * if passed 0 wil return first node of the tree (root)
+ * Retrieves next node, skipping all the children nodes of the pointed node,
+ * returns pointer to next node, no matter if it exists or not.
+ */
+void *
+fdt_skip_node(void *node)
+{
+ u_int32_t *ptr = node;
+
+ if (!tree_inited || !node)
+ return NULL;
+
+ if (betoh32(*ptr) != FDT_NODE_BEGIN)
+ return NULL;
+
+ ptr++;
+
+ ptr = skip_node_name(ptr);
+ ptr = skip_props(ptr);
+
+ /* skip children */
+ while (betoh32(*ptr) == FDT_NODE_BEGIN)
+ ptr = fdt_skip_node(ptr);
+
+ return (betoh32(*ptr) == FDT_NODE_END) ? (ptr + 1) : NULL;
+}
+
+/*
+ * Retrieves next node, skipping all the children nodes of the pointed node,
+ * returns pointer to next node if it exists, otherwise returns NULL.
+ * If passed 0 will return first node of the tree (root).
*/
void *
fdt_next_node(void *node)
@@ -199,9 +228,15 @@ fdt_next_node(void *node)
/* skip children */
while (betoh32(*ptr) == FDT_NODE_BEGIN)
- ptr = fdt_next_node(ptr);
+ ptr = fdt_skip_node(ptr);
- return (betoh32(*ptr) == FDT_NODE_END) ? (ptr + 1) : NULL;
+ if (betoh32(*ptr) != FDT_NODE_END)
+ return NULL;
+
+ if (betoh32(*(ptr + 1)) != FDT_NODE_BEGIN)
+ return NULL;
+
+ return (ptr + 1);
}
/*
>
> > This diff makes the function return NULL if the token behind the
> > child pointer is not FDT_NODE_BEGIN. This tells us we reached
> > the end of the list and we have not found a child matching the
> > passed name(s). Thus the find has failed.
> >
> > Patrick
> >
> > diff --git sys/arch/socppc/socppc/fdt.c sys/arch/socppc/socppc/fdt.c
> > index 0dec4fb..741763c 100644
> > --- sys/arch/socppc/socppc/fdt.c
> > +++ sys/arch/socppc/socppc/fdt.c
> > @@ -274,6 +274,13 @@ fdt_find_node(char *name)
> >
> > for (child = fdt_child_node(node); child;
> > child = fdt_next_node(child)) {
> > + /*
> > + * A child always starts with a FDT_NODE_BEGIN token.
> > + * If it's another token, we have reached the end of
> > + * the list but have not found a match.
> > + */
> > + if (betoh32(*(uint32_t *)child) != FDT_NODE_BEGIN)
> > + return NULL;
> > if (strncmp(p, fdt_node_name(child), q - p) == 0) {
> > node = child;
> > break;
> >
>
