[PATCH] gst-tool: Fix ASAN issue on comparing options

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] gst-tool: Fix ASAN issue on comparing options

Holger Freyther
From: Holger Hans Peter Freyther <[hidden email]>

In case the name is longer than all_opts->name we would try to
read past the string. Start using strncmp and strlen to make
sure to fully consume all_opts->name and don't read out of
bounds.

2017-02-08  Holger Hans Peter Freyther  <[hidden email]>

        * gst-tool.c: Use strncmp instead of memcmp.
---
 ChangeLog  | 4 ++++
 gst-tool.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index a40b68d..0542be5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-02-08  Holger Hans Peter Freyther  <[hidden email]>
+
+ * gst-tool.c: Use strncmp instead of memcmp.
+
 2015-11-07  Holger Hans Peter Freyther  <[hidden email]>
 
  * build-aux/overflow-builtins.m4: Add new macro.
diff --git a/gst-tool.c b/gst-tool.c
index 1739793..8d817c4 100644
--- a/gst-tool.c
+++ b/gst-tool.c
@@ -381,7 +381,7 @@ parse_long_option (const char *name, const char *arg)
     len = p++ - name;
 
   for (all_opts = long_opts; all_opts; all_opts = all_opts->next)
-    if (!memcmp (name, all_opts->name, len))
+    if (strlen(all_opts->name) >= len && !strncmp (name, all_opts->name, len))
       {
  opt = all_opts;
  if (opt->name[len] == '\0')
--
2.10.2


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gst-tool: Fix ASAN issue on comparing options

Holger Freyther

> On 8 Feb 2017, at 21:50, Holger Freyther <[hidden email]> wrote:
>


Hi!

> In case the name is longer than all_opts->name we would try to
> read past the string. Start using strncmp and strlen to make
> sure to fully consume all_opts->name and don't read out of
> bounds.

my inbox can't find this but I think we had this before? It is
triggered by ASAN right away.




>
> 2017-02-08  Holger Hans Peter Freyther  <[hidden email]>
>
> * gst-tool.c: Use strncmp instead of memcmp.
> ---
> ChangeLog  | 4 ++++
> gst-tool.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index a40b68d..0542be5 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-02-08  Holger Hans Peter Freyther  <[hidden email]>
> +
> + * gst-tool.c: Use strncmp instead of memcmp.
> +
> 2015-11-07  Holger Hans Peter Freyther  <[hidden email]>
>
> * build-aux/overflow-builtins.m4: Add new macro.
> diff --git a/gst-tool.c b/gst-tool.c
> index 1739793..8d817c4 100644
> --- a/gst-tool.c
> +++ b/gst-tool.c
> @@ -381,7 +381,7 @@ parse_long_option (const char *name, const char *arg)
>     len = p++ - name;
>
>   for (all_opts = long_opts; all_opts; all_opts = all_opts->next)
> -    if (!memcmp (name, all_opts->name, len))
> +    if (strlen(all_opts->name) >= len && !strncmp (name, all_opts->name, len))
>       {
> opt = all_opts;
> if (opt->name[len] == '\0')
> --
> 2.10.2
>
>
> _______________________________________________
> help-smalltalk mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/help-smalltalk


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gst-tool: Fix ASAN issue on comparing options

Paolo Bonzini-2


On 08/02/2017 15:51, Holger Freyther wrote:

>
>> On 8 Feb 2017, at 21:50, Holger Freyther <[hidden email]> wrote:
>>
>
>
> Hi!
>
>> In case the name is longer than all_opts->name we would try to
>> read past the string. Start using strncmp and strlen to make
>> sure to fully consume all_opts->name and don't read out of
>> bounds.
>
> my inbox can't find this but I think we had this before? It is
> triggered by ASAN right away.


Wouldn't the '\0' mismatch first?

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gst-tool: Fix ASAN issue on comparing options

Holger Freyther

> On 12. Feb 2017, at 13:16, Holger Freyther <[hidden email]> wrote:
>

Dear Paolo,


>> Wouldn't the '\0' mismatch first?
>
> "Both strings are assumed to be n bytes long". I guess an optimized memcmp will fetch 32/64 bytes at a time (manual loop unrolling?).


I noticed I didn't push this change yet. I think in the case ASAN reported, name and all_opts->name are of different size and 1-3 bytes after the \0 will be fetched? If I see this correctly right now we want to check if name is a prefix of all_opts->name? For this to be true all_opts->name must be at least as long as the prefix (name)? Am I wrong?


holger
_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gst-tool: Fix ASAN issue on comparing options

Paolo Bonzini-2
On 24/11/2017 10:30, Holger Freyther wrote:

>
>> On 12. Feb 2017, at 13:16, Holger Freyther <[hidden email]> wrote:
>>
>
> Dear Paolo,
>
>
>>> Wouldn't the '\0' mismatch first?
>>
>> "Both strings are assumed to be n bytes long". I guess an optimized memcmp will fetch 32/64 bytes at a time (manual loop unrolling?).
>
>
> I noticed I didn't push this change yet. I think in the case ASAN
> reported, name and all_opts->name are of different size and 1-3 bytes
> after the \0 will be fetched? If I see this correctly right now we want
> to check if name is a prefix of all_opts->name? For this to be true
> all_opts->name must be at least as long as the prefix (name)? Am I wrong?

Lee is right, see also https://trust-in-soft.com/memcmp-requires-pointers-to-fully-valid-buffers/

However, I am not sure strlen is needed.  The code is:


  if (!p)
    len = strlen (name);
  else
    len = p++ - name;

  for (all_opts = long_opts; all_opts; all_opts = all_opts->next)
-   if (!memcmp (name, all_opts->name, len))
+   if (strlen(all_opts->name) >= len && !strncmp (name, all_opts->name, len))

so len <= strlen(name): there is no NULL byte in the first LEN
bytes of NAME.  For the first LEN bytes to be equal in the two
arguments to strncmp, there must be no NULL byte in all_opts->name,
and thus strlen(all_opts->name) >= len too.

Thanks,

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk