Re: [media-ctl PATCH] Compare entity name length aswell

["Aguirre, Sergio" <saaguirre@xxxxxx>]

Hi Laurent,

On Sun, Apr 29, 2012 at 6:13 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Sergio,
>
> On Sunday 29 April 2012 17:36:43 Aguirre, Sergio wrote:
>> On Sun, Apr 29, 2012 at 12:40 PM, Laurent Pinchart wrote:
>> > On Saturday 28 April 2012 10:04:01 Aguirre, Sergio wrote:
>> >> On Wed, Apr 25, 2012 at 8:57 AM, Sergio Aguirre <saaguirre@xxxxxx> wrote:
>> >> > Otherwise, some false positives might arise when
>> >> > having 2 subdevices with similar names, like:
>> >> >
>> >> > "OMAP4 ISS ISP IPIPEIF"
>> >> > "OMAP4 ISS ISP IPIPE"
>> >> >
>> >> > Before this patch, trying to find "OMAP4 ISS ISP IPIPE", resulted
>> >> > in a false entity match, retrieving "OMAP4 ISS ISP IPIPEIF"
>> >> > information instead.
>> >> >
>> >> > Checking length should ensure such cases are handled well.
>> >>
>> >> Any feedback about this?
>> >
>> > Thanks for the patch, and sorry for the delay.
>>
>> No problem. :)
>>
>> >> > Signed-off-by: Sergio Aguirre <saaguirre@xxxxxx>
>> >> > ---
>> >> >  src/mediactl.c |    3 ++-
>> >> >  1 files changed, 2 insertions(+), 1 deletions(-)
>> >> >
>> >> > diff --git a/src/mediactl.c b/src/mediactl.c
>> >> > index 5b8c587..451a386 100644
>> >> > --- a/src/mediactl.c
>> >> > +++ b/src/mediactl.c
>> >> > @@ -66,7 +66,8 @@ struct media_entity *media_get_entity_by_name(struct
>> >> > media_device *media, for (i = 0; i < media->entities_count; ++i) {
>> >> >                struct media_entity *entity = &media->entities[i];
>> >> >
>> >> > -               if (strncmp(entity->info.name, name, length) == 0)
>> >> > +               if ((strncmp(entity->info.name, name, length) == 0) &&
>> >> > +                   (strlen(entity->info.name) == length))
>> >> >                        return entity;
>> >> >        }
>> >
>> > Instead of calling strlen() which has a O(n) complexity, what about just
>> > checking that the entity name has a '\0' in the length'th position ?
>> > Something like the following patch:
>> >
>> > From 46bec667b675573cf1ce698c68112e3dbd31930e Mon Sep 17 00:00:00 2001
>> > From: Sergio Aguirre <saaguirre@xxxxxx>
>> > Date: Wed, 25 Apr 2012 08:57:13 -0500
>> > Subject: [PATCH] Compare name length to avoid false positives in
>> > media_get_entity_by_name
>> >
>> > If two subdevice have names that only differ by a suffix (such as "OMAP4
>> > ISS ISP IPIPE" and "OMAP4 ISS ISP IPIPEIF") the media_get_entity_by_name
>> > function might return a pointer to the entity with the longest name when
>> > called with the shortest name. Fix this by verifying that the candidate
>> > entity name length is equal to the requested name length.
>> >
>> > Signed-off-by: Sergio Aguirre <saaguirre@xxxxxx>
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> > ---
>> >  src/mediactl.c |    9 ++++++++-
>> >  src/tools.h    |    1 +
>> >  2 files changed, 9 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/src/mediactl.c b/src/mediactl.c
>> > index 5b8c587..bc6a713 100644
>> > --- a/src/mediactl.c
>> > +++ b/src/mediactl.c
>> > @@ -63,10 +63,17 @@ struct media_entity *media_get_entity_by_name(struct
>> > media_device *media,
>> >  {
>> >        unsigned int i;
>> >
>> > +       /* A match is impossible if the entity name is longer than the
>> > maximum +        * size we can get from the kernel.
>> > +        */
>> > +       if (length >= FIELD_SIZEOF(struct media_entity_desc, name))
>> > +               return NULL;
>> > +
>>
>> Good idea.
>>
>> >        for (i = 0; i < media->entities_count; ++i) {
>> >                struct media_entity *entity = &media->entities[i];
>> >
>> > -               if (strncmp(entity->info.name, name, length) == 0)
>> > +               if (strncmp(entity->info.name, name, length) == 0 &&
>> > +                   entity->info.name[length] == '\0')
>>
>> ACK.
>>
>> Your patch is definitely better.
>>
>> IMHO, I think it'll be more fair if you put yourself as the author of this
>> new patch, and me as: "Reported-by:".
>
> It's such a small patch, it's fine :-) Thank you for the proposal nonetheless.
>
> I've pushed the patch to the repository.

Excellent! Thanks for that :)

Regards,
Sergio

>
>> >                        return entity;
>> >        }
>> >
>> > diff --git a/src/tools.h b/src/tools.h
>> > index e56edb2..de06cb3 100644
>> > --- a/src/tools.h
>> > +++ b/src/tools.h
>> > @@ -23,6 +23,7 @@
>> >  #define __TOOLS_H__
>> >
>> >  #define ARRAY_SIZE(array)      (sizeof(array) / sizeof((array)[0]))
>> > +#define FIELD_SIZEOF(t, f)     (sizeof(((t*)0)->f))
>> >
>> >  #endif /* __TOOLS_H__ */
>> >
>
> --
> Regards,
>
> Laurent Pinchart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html