[PATCH v7] string: Adds tests for test-strncasecmp and test-strncpy

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v7] string: Adds tests for test-strncasecmp and test-strncpy

Sourceware - libc-alpha mailing list
Changes since v6:
        - Fixed english spelling.
        - Shrunk s2 size by 2 on string/test-strncpy.c.

--- >8 ---

Adds tests to check if strings placed at page bondaries are
handled correctly by strncasecmp and strncpy similar to tests
for strncmp and strnlen.
---
 string/test-strncasecmp.c | 43 +++++++++++++++++++++++++++++++++++++++
 string/test-strncpy.c     | 35 +++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
index 6a9c27beae..502222ed1d 100644
--- a/string/test-strncasecmp.c
+++ b/string/test-strncasecmp.c
@@ -137,6 +137,48 @@ do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,
     do_one_test (impl, s1, s2, n, exp_result);
 }
 
+static void
+do_page_tests (void)
+{
+  char *s1, *s2;
+  int exp_result;
+  const size_t maxoffset = 64;
+
+  s1 = (char *) buf1 + BUF1PAGES * page_size - maxoffset;
+  memset (s1, 'a', maxoffset - 1);
+  s1[maxoffset - 1] = '\0';
+
+  s2 = (char *) buf2 + page_size - maxoffset;
+  memset (s2, 'a', maxoffset - 1);
+  s2[maxoffset - 1] = '\0';
+
+  /* At this point s1 and s2 point to distinct memory regions containing
+     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
+     page with read/write access and the next page is protected with PROT_NONE
+     (meaning that any access outside of the page regions will trigger an
+     invalid memory access).
+
+     The loop checks for all possible offsets up to maxoffset for both
+     inputs with a size larger than the string (so memory access outside
+     the expected memory regions might trigger invalid access).  */
+
+  for (size_t off1 = 0; off1 < maxoffset; off1++)
+    {
+      for (size_t off2 = 0; off2 < maxoffset; off2++)
+ {
+  exp_result = (off1 == off2)
+ ? 0
+ : off1 < off2
+  ? 'a'
+  : -'a';
+
+  FOR_EACH_IMPL (impl, 0)
+    check_result (impl, s1 + off1, s2 + off2, maxoffset + 1,
+  exp_result);
+ }
+    }
+}
+
 static void
 do_random_tests (void)
 {
@@ -334,6 +376,7 @@ test_locale (const char *locale)
     }
 
   do_random_tests ();
+  do_page_tests ();
 }
 
 int
diff --git a/string/test-strncpy.c b/string/test-strncpy.c
index c978753ad8..2919bbe181 100644
--- a/string/test-strncpy.c
+++ b/string/test-strncpy.c
@@ -155,6 +155,40 @@ do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
     do_one_test (impl, s2, s1, len, n);
 }
 
+static void
+do_page_tests (void)
+{
+  CHAR *s1, *s2;
+  const size_t maxoffset = 64;
+
+  /* Put s1 at the edge of buf1's last page.  */
+  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
+  /* Put s2 almost at the edge of buf2, it needs room to put a string with
+     size of maxoffset + 1 at s2 + maxoffset.  */
+  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;
+
+  MEMSET (s1, 'a', maxoffset - 1);
+  s1[maxoffset - 1] = '\0';
+
+  /* Both strings are bounded to a page with read/write access and the next
+     page is protected with PROT_NONE (meaning that any access outside of the
+     page regions will trigger an invalid memory access).
+
+     The loop copies the string s1 for all possible offsets up to maxoffset
+     for both inputs with a size larger than s1 (so memory access outside the
+     expected memory regions might trigger invalid access).  */
+
+  for (size_t off1 = 0; off1 < maxoffset; off1++)
+    {
+      for (size_t off2 = 0; off2 < maxoffset; off2++)
+ {
+  FOR_EACH_IMPL (impl, 0)
+    do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,
+ maxoffset + 1);
+ }
+    }
+}
+
 static void
 do_random_tests (void)
 {
@@ -317,6 +351,7 @@ test_main (void)
     }
 
   do_random_tests ();
+  do_page_tests ();
   return ret;
 }
 
--
2.26.2

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v7] string: Adds tests for test-strncasecmp and test-strncpy

Sourceware - libc-alpha mailing list
Hi Raphael.
I just have a little comment down there. The rest of the patch is good
for me.

Quoting Raphael Moreira Zinsly (2020-07-15 10:16:31)

> Changes since v6:
>         - Fixed english spelling.
>         - Shrunk s2 size by 2 on string/test-strncpy.c.
>
> --- >8 ---
>
> Adds tests to check if strings placed at page bondaries are
> handled correctly by strncasecmp and strncpy similar to tests
> for strncmp and strnlen.
> ---
>  string/test-strncasecmp.c | 43 +++++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 35 +++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
>
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..502222ed1d 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,48 @@ do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,
>      do_one_test (impl, s1, s2, n, exp_result);
>  }
>  
> +static void
> +do_page_tests (void)
> +{
> +  char *s1, *s2;
> +  int exp_result;
> +  const size_t maxoffset = 64;
> +
> +  s1 = (char *) buf1 + BUF1PAGES * page_size - maxoffset;
> +  memset (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  s2 = (char *) buf2 + page_size - maxoffset;
> +  memset (s2, 'a', maxoffset - 1);
> +  s2[maxoffset - 1] = '\0';
> +
> +  /* At this point s1 and s2 point to distinct memory regions containing
> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
> +     page with read/write access and the next page is protected with PROT_NONE
> +     (meaning that any access outside of the page regions will trigger an
> +     invalid memory access).
> +
> +     The loop checks for all possible offsets up to maxoffset for both
> +     inputs with a size larger than the string (so memory access outside
> +     the expected memory regions might trigger invalid access).  */
> +
> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < maxoffset; off2++)
> +       {
> +         exp_result = (off1 == off2)
> +                       ? 0
> +                       : off1 < off2
> +                         ? 'a'
> +                         : -'a';
> +
> +         FOR_EACH_IMPL (impl, 0)
> +           check_result (impl, s1 + off1, s2 + off2, maxoffset + 1,
> +                         exp_result);
> +       }
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -334,6 +376,7 @@ test_locale (const char *locale)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>  }
>  
>  int
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..2919bbe181 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,40 @@ do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
>      do_one_test (impl, s2, s1, len, n);
>  }
>  
> +static void
> +do_page_tests (void)
> +{
> +  CHAR *s1, *s2;
> +  const size_t maxoffset = 64;
> +
> +  /* Put s1 at the edge of buf1's last page.  */
> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
> +  /* Put s2 almost at the edge of buf2, it needs room to put a string with
> +     size of maxoffset + 1 at s2 + maxoffset.  */
> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;
> +
It seams to me that this should be
s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - (maxoffset *2 + 1);
As if you are at point - (maxoffset * 2 + 1) and sum maxoffset you be
leaved at point - (maxoffset + 1) there for with size (maxoffset + 1).
2 * maxoffset  + 1 - maxoffset = maxoffset +1

> +  MEMSET (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  /* Both strings are bounded to a page with read/write access and the next
> +     page is protected with PROT_NONE (meaning that any access outside of the
> +     page regions will trigger an invalid memory access).
> +
> +     The loop copies the string s1 for all possible offsets up to maxoffset
> +     for both inputs with a size larger than s1 (so memory access outside the
> +     expected memory regions might trigger invalid access).  */
> +
> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < maxoffset; off2++)
> +       {
> +         FOR_EACH_IMPL (impl, 0)
> +           do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,
> +                        maxoffset + 1);
> +       }
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -317,6 +351,7 @@ test_main (void)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>    return ret;
>  }
>  
> --
> 2.26.2
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v7] string: Adds tests for test-strncasecmp and test-strncpy

Sourceware - libc-alpha mailing list
Hi Lucas, thanks for reviewing again, the answer is bellow:

On 20/07/2020 09:30, Lucas A. M. Magalhaes wrote:

> Hi Raphael.
> I just have a little comment down there. The rest of the patch is good
> for me.
>...
>> +static void
>> +do_page_tests (void)
>> +{
>> +  CHAR *s1, *s2;
>> +  const size_t maxoffset = 64;
>> +
>> +  /* Put s1 at the edge of buf1's last page.  */
>> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
>> +  /* Put s2 almost at the edge of buf2, it needs room to put a string with
>> +     size of maxoffset + 1 at s2 + maxoffset.  */
>> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;
>> +
> It seams to me that this should be
> s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - (maxoffset *2 + 1);
> As if you are at point - (maxoffset * 2 + 1) and sum maxoffset you be
> leaved at point - (maxoffset + 1) there for with size (maxoffset + 1).
> 2 * maxoffset  + 1 - maxoffset = maxoffset +1
>

We are testing with maxoffset+1 to try with a size larger than the
string, the string itself has size maxoffset (maxoffset-1 + \0) as you
can see bellow:

 >> +  MEMSET (s1, 'a', maxoffset - 1);
 >> +  s1[maxoffset - 1] = '\0';

So you already have enough room for the entire string at the point with
max off2 regardless of the size passed to strncpy.

Best Regards,
--
Raphael Moreira Zinsly
IBM
Linux on Power Toolchain
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v7] string: Adds tests for test-strncasecmp and test-strncpy

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
Hi Raphael,

On 7/15/20 10:16 AM, Raphael Moreira Zinsly via Libc-alpha wrote:

> Changes since v6:
> - Fixed english spelling.
> - Shrunk s2 size by 2 on string/test-strncpy.c.
>
> --- >8 ---
>
> Adds tests to check if strings placed at page bondaries are
> handled correctly by strncasecmp and strncpy similar to tests
> for strncmp and strnlen.
> ---
>  string/test-strncasecmp.c | 43 +++++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 35 +++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
>
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..502222ed1d 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,48 @@ do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,
>      do_one_test (impl, s1, s2, n, exp_result);
>  }
>  
> +static void
> +do_page_tests (void)
> +{
> +  char *s1, *s2;
> +  int exp_result;
> +  const size_t maxoffset = 64;
> +
> +  s1 = (char *) buf1 + BUF1PAGES * page_size - maxoffset;
> +  memset (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  s2 = (char *) buf2 + page_size - maxoffset;
> +  memset (s2, 'a', maxoffset - 1);
> +  s2[maxoffset - 1] = '\0';

Ok. Place strings on the border of the pages.

> +
> +  /* At this point s1 and s2 point to distinct memory regions containing
> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
> +     page with read/write access and the next page is protected with PROT_NONE
> +     (meaning that any access outside of the page regions will trigger an
> +     invalid memory access).
> +
> +     The loop checks for all possible offsets up to maxoffset for both
> +     inputs with a size larger than the string (so memory access outside
> +     the expected memory regions might trigger invalid access).  */

Good comment. Makes the code way simpler to read.

> +
> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < maxoffset; off2++)

Ok. Comparing the strings at all possible offsets.

> + {
> +  exp_result = (off1 == off2)
> + ? 0
> + : off1 < off2
> +  ? 'a'
> +  : -'a';
> +
> +  FOR_EACH_IMPL (impl, 0)
> +    check_result (impl, s1 + off1, s2 + off2, maxoffset + 1,

Ok. Set N so that 1 byte is on the protected page to try to catch buggy
implementations.

> +  exp_result);
> + }
> +    }
> +}> +
>  static void
>  do_random_tests (void)
>  {
> @@ -334,6 +376,7 @@ test_locale (const char *locale)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>  }

Ok. Run the test for all the different locales being tested.

>  
>  int
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..2919bbe181 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,40 @@ do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
>      do_one_test (impl, s2, s1, len, n);
>  }
>  
> +static void
> +do_page_tests (void)
> +{
> +  CHAR *s1, *s2;
> +  const size_t maxoffset = 64;
> +
> +  /* Put s1 at the edge of buf1's last page.  */
> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
> +  /* Put s2 almost at the edge of buf2, it needs room to put a string with
> +     size of maxoffset + 1 at s2 + maxoffset.  */

This comment may be slightly misleading. The room from s2 + maxoffset to
the end of the page is maxoffset, so not enough for a string of size
maxoffset + 1. That last value will be used for N when calling strncpy,
but the string is actually shorter.

> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;

> +
> +  MEMSET (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  /* Both strings are bounded to a page with read/write access and the next
> +     page is protected with PROT_NONE (meaning that any access outside of the
> +     page regions will trigger an invalid memory access).
> +
> +     The loop copies the string s1 for all possible offsets up to maxoffset
> +     for both inputs with a size larger than s1 (so memory access outside the
> +     expected memory regions might trigger invalid access).  */
> +
> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < maxoffset; off2++)
> + {
> +  FOR_EACH_IMPL (impl, 0)
> +    do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,

Ok. Had a hard time following the combination of offsets for each call,
but it looks fine. The maximum size of the string to be copied will be
63 (maxoffset - off1 - 1, when off1 is 0), because \0 will be at the
last byte of the page.

> + maxoffset + 1);
> + }
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -317,6 +351,7 @@ test_main (void)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>    return ret;
>  }
>  
>

I would suggest changing only that one comment, the rest LGTM.

Thanks,
Matheus Castanho