* [media-ctl PATCH] Compare entity name length aswell @ 2012-04-25 13:57 Sergio Aguirre 2012-04-28 15:04 ` Aguirre, Sergio 0 siblings, 1 reply; 6+ messages in thread From: Sergio Aguirre @ 2012-04-25 13:57 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, Sergio Aguirre 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. Signed-off-by: Sergio Aguirre <saaguirre@ti.com> --- 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; } -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [media-ctl PATCH] Compare entity name length aswell 2012-04-25 13:57 [media-ctl PATCH] Compare entity name length aswell Sergio Aguirre @ 2012-04-28 15:04 ` Aguirre, Sergio 2012-04-29 17:40 ` Laurent Pinchart 0 siblings, 1 reply; 6+ messages in thread From: Aguirre, Sergio @ 2012-04-28 15:04 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, Sergio Aguirre Hi Laurent On Wed, Apr 25, 2012 at 8:57 AM, Sergio Aguirre <saaguirre@ti.com> 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? Regards, Sergio > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > --- > 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; > } > > -- > 1.7.5.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [media-ctl PATCH] Compare entity name length aswell 2012-04-28 15:04 ` Aguirre, Sergio @ 2012-04-29 17:40 ` Laurent Pinchart 2012-04-29 22:36 ` Aguirre, Sergio 0 siblings, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2012-04-29 17:40 UTC (permalink / raw) To: Aguirre, Sergio; +Cc: linux-media Hi Sergio, On Saturday 28 April 2012 10:04:01 Aguirre, Sergio wrote: > On Wed, Apr 25, 2012 at 8:57 AM, Sergio Aguirre <saaguirre@ti.com> 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. > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > > --- > > 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@ti.com> 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@ti.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- 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; + 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') 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [media-ctl PATCH] Compare entity name length aswell 2012-04-29 17:40 ` Laurent Pinchart @ 2012-04-29 22:36 ` Aguirre, Sergio 2012-04-29 23:13 ` Laurent Pinchart 0 siblings, 1 reply; 6+ messages in thread From: Aguirre, Sergio @ 2012-04-29 22:36 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media Hi Laurent, On Sun, Apr 29, 2012 at 12:40 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Sergio, > > On Saturday 28 April 2012 10:04:01 Aguirre, Sergio wrote: >> On Wed, Apr 25, 2012 at 8:57 AM, Sergio Aguirre <saaguirre@ti.com> 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@ti.com> >> > --- >> > 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@ti.com> > 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@ti.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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:". 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [media-ctl PATCH] Compare entity name length aswell 2012-04-29 22:36 ` Aguirre, Sergio @ 2012-04-29 23:13 ` Laurent Pinchart 2012-04-29 23:18 ` Aguirre, Sergio 0 siblings, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2012-04-29 23:13 UTC (permalink / raw) To: Aguirre, Sergio; +Cc: linux-media 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@ti.com> 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@ti.com> > >> > --- > >> > 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@ti.com> > > 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@ti.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > 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. > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [media-ctl PATCH] Compare entity name length aswell 2012-04-29 23:13 ` Laurent Pinchart @ 2012-04-29 23:18 ` Aguirre, Sergio 0 siblings, 0 replies; 6+ messages in thread From: Aguirre, Sergio @ 2012-04-29 23:18 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media Hi Laurent, On Sun, Apr 29, 2012 at 6:13 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@ti.com> 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@ti.com> >> >> > --- >> >> > 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@ti.com> >> > 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@ti.com> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> > --- >> > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-29 23:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-25 13:57 [media-ctl PATCH] Compare entity name length aswell Sergio Aguirre 2012-04-28 15:04 ` Aguirre, Sergio 2012-04-29 17:40 ` Laurent Pinchart 2012-04-29 22:36 ` Aguirre, Sergio 2012-04-29 23:13 ` Laurent Pinchart 2012-04-29 23:18 ` Aguirre, Sergio
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.