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

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

> 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.





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

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

Wouldn't the '\0' mismatch first?

Paolo

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

> 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

On 24/11/2017 10:30, Holger Freyther wrote:
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