On Wed, Sep 19, 2018 at 09:19:14AM -0700, Rob Herring wrote:
> On Tue, Sep 18, 2018 at 9:38 PM David Gibson
> <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Sep 18, 2018 at 04:46:38PM -0700, Rob Herring wrote:
> > > Since commit 7975f6422260 ("Fix widespread incorrect use of strneq(),
> > > replace with new strprefixeq()") simple-bus checks have been silently
> > > skipped. The problem was 'end - str' is one more than the string length
> > > and the strnlen in strprefixeq fails. This can't be fixed simply by
> > > subtracting one as it is possible to have multiple '\0' at the end of
> > > the property. So first check the property is null terminated and then
> > > use streq() for comparisons.
> > >
> > > Add some tests so the problem doesn't happen again.
> > >
> > > Fixes: 7975f6422260 ("Fix widespread incorrect use of strneq(), replace with new strprefixeq()")
> > > Reported-by: Kumar Gala <kumar.gala@xxxxxxxxxx>
> > > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> >
> > So, the existing version was definitely wrong. The second parameter
> > to strprefixeq needs to be the exact length of the prefix to compare,
> > and it's clearly not here. But your proposed version isn't quite
> > right either.
> >
> > > ---
> > > checks.c | 8 ++++++--
> > > tests/run_tests.sh | 4 ++++
> > > tests/unit-addr-simple-bus-compatible.dts | 18 ++++++++++++++++++
> > > tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++++++++++++++
> > > 4 files changed, 46 insertions(+), 2 deletions(-)
> > > create mode 100644 tests/unit-addr-simple-bus-compatible.dts
> > > create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
> > >
> > > diff --git a/checks.c b/checks.c
> > > index 9c9b0c328af6..1ccbfa60c839 100644
> > > --- a/checks.c
> > > +++ b/checks.c
> > > @@ -908,9 +908,13 @@ static bool node_is_compatible(struct node *node, const char *compat)
> > > if (!prop)
> > > return false;
> > >
> > > - for (str = prop->val.val, end = str + prop->val.len; str < end;
> > > + str = prop->val.val;
> > > + if (strnlen(str, prop->val.len) == prop->val.len)
> > > + return false;
> >
> > This will catch the case where the property has no \0s at all, but not
> > a case like say...
> > compatible = "foo", /bits/ 8 <'s' 'i' 'm'>;
>
> That's just crap.
Well, yes, that's kind of the point.
> We do also validate already that compatible is a
> string list. Not sure offhand if that's a dependency for this.
Ah.. good point. If we add a dependency for that, your original
should be fine - and you can even drop that initial check.
> Maybe I should just check that the last byte is null.
Adding a dependency on the stringlist check seems better to me (and
amounts to almost the same thing).
> > > +
> > > + for (end = str + prop->val.len; str < end;
> > > str += strnlen(str, end - str) + 1) {
> > > - if (strprefixeq(str, end - str, compat))
> > > + if (streq(str, compat))
> >
> > ...which will lead to this streq() running over the end of the property.
> >
> > I think you instead will need something like this:
> > len = strlen(compat);
> > /* ... */
> > if ((end - str) > len && (memcmp(str, compat, len) == 0))
> > /* ... */
>
> That would match if say we have "simple-bus-foo" and compat is
> "simple-bus". The memcmp would need to be len + 1, but then that is
> just back to strcmp aka streq.
Yeah, I meant len + 1. When the length is known in advance, I
generally prefer memcmp to strcmp().
--
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
Attachment:
signature.asc
Description: PGP signature
