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

Changes since v5:
string/test-strncpy.c:
	- Adjusted s1 and s2 placements.
	- Used the same pattern as test-strncasecmp.

--- >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 | 44 +++++++++++++++++++++++++++++++++++++++
 string/test-strncpy.c     | 36 ++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

On Wed, Jul 08, 2020 at 05:20:09PM -0300, Raphael Moreira Zinsly wrote:
> Changes since v5:
> string/test-strncpy.c:
> 	- Adjusted s1 and s2 placements.
> 	- Used the same pattern as test-strncasecmp.
> 
> --- >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 | 44 +++++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 36 ++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..628135b962 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,49 @@ 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;
> +  /* Assumes up to 512-bit wide read/stores.  */

Consider more explanation here.

> +  const size_t maxoffset = 64;
> +
> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;

You shouldn't need the parens around BUF1PAGES.

> +  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 points to distinct memory regions containing

nit (English): s/points/point/

> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
> +     page with write/read access and the next page is protected with PROT_NONE

nit, and totally subjective, but since I'm here, it's more common to use "read/write".

> +     (meaning that any access outside of the page regions will trigger an
> +     invalid memory access).
> +
> +     The loop checks for all possible offset up to maxoffset for both

nit: s/offset/offsets/

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

nit, subjective: consider parens around "off1 == off2" for readability.

> +			? 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 +377,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..c8d4ad58a6 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,41 @@ 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;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  const size_t maxoffset = 64;
> +
> +  /* Put s1 at the edge of buf1's last page.  */
> +  s1 = (CHAR *) buf1 + (BUF1PAGES) * (page_size / sizeof(CHAR)) - maxoffset;

Can remove parens around BUF1PAGES and "page_size / sizeof(CHAR)".

> +  /* 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 + 1) * 2;

Could this be made more similar to the above computation for s1?
Why the "* 2"?  I guess there is no definition for BUF2PAGES.  Ugh.
I propose adding one, to avoid magic numbers.

> +
> +  MEMSET (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  /* Both strings are bounded to a page with write/read access and the next

nit, and totally subjective, but since I'm here, it's more common to use "read/write".

> +     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 offset up to maxoffset

nit: s/offset/offsets/

> +     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 +352,7 @@ test_main (void)
>      }
> 
>    do_random_tests ();
> +  do_page_tests ();
>    return ret;
>  }

LGTM with the above addressed.

PC

Hi Raphael,
Thanks fo the patch.

Quoting Raphael Moreira Zinsly (2020-07-08 17:20:09)
> Changes since v5:
> string/test-strncpy.c:
>         - Adjusted s1 and s2 placements.
>         - Used the same pattern as test-strncasecmp.
> 
> --- >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 | 44 +++++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 36 ++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..628135b962 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,49 @@ 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;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  const size_t maxoffset = 64;
> +
> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
> +  memset (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';

OK.
> +
> +  s2 = (char *) buf2 + page_size - maxoffset;
> +  memset (s2, 'a', maxoffset - 1);
> +  s2[maxoffset - 1] = '\0';
> +
OK. buf2 is only 1 page size.

> +  /* At this point s1 and s2 points to distinct memory regions containing
> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
> +     page with write/read 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 offset 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);
> +       }
> +    }
> +}
> +

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

OK.
>  int
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..c8d4ad58a6 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,41 @@ 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;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  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 + 1) * 2;
> +

IDK if was the intention. If you add maxoffset to s2 it will still leave
maxoffset + 2 bytes for s2 and not maxoffset as the comment says.

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

OK.

> +  /* Both strings are bounded to a page with write/read 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 offset 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);

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

OK.
> -- 
> 2.26.2
>

On 09/07/2020 12:27, Paul A. Clarke wrote:
> On Wed, Jul 08, 2020 at 05:20:09PM -0300, Raphael Moreira Zinsly wrote:
>> Changes since v5:
>> string/test-strncpy.c:
>> 	- Adjusted s1 and s2 placements.
>> 	- Used the same pattern as test-strncasecmp.
>>
>> --- >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 | 44 +++++++++++++++++++++++++++++++++++++++
>>   string/test-strncpy.c     | 36 ++++++++++++++++++++++++++++++++
>>   2 files changed, 80 insertions(+)
>>
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
>> index 6a9c27beae..628135b962 100644
>> --- a/string/test-strncasecmp.c
>> +++ b/string/test-strncasecmp.c
>> @@ -137,6 +137,49 @@ 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;
>> +  /* Assumes up to 512-bit wide read/stores.  */
> 
> Consider more explanation here.
> 
>> +  const size_t maxoffset = 64;
>> +
>> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
> 
> You shouldn't need the parens around BUF1PAGES.
> 
>> +  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 points to distinct memory regions containing
> 
> nit (English): s/points/point/
> 
>> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
>> +     page with write/read access and the next page is protected with PROT_NONE
> 
> nit, and totally subjective, but since I'm here, it's more common to use "read/write".
> 
>> +     (meaning that any access outside of the page regions will trigger an
>> +     invalid memory access).
>> +
>> +     The loop checks for all possible offset up to maxoffset for both
> 
> nit: s/offset/offsets/
> 
>> +     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
> 
> nit, subjective: consider parens around "off1 == off2" for readability.
> 
>> +			? 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 +377,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..c8d4ad58a6 100644
>> --- a/string/test-strncpy.c
>> +++ b/string/test-strncpy.c
>> @@ -155,6 +155,41 @@ 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;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  const size_t maxoffset = 64;
>> +
>> +  /* Put s1 at the edge of buf1's last page.  */
>> +  s1 = (CHAR *) buf1 + (BUF1PAGES) * (page_size / sizeof(CHAR)) - maxoffset;
> 
> Can remove parens around BUF1PAGES and "page_size / sizeof(CHAR)".
> 
>> +  /* 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 + 1) * 2;
> 
> Could this be made more similar to the above computation for s1?
> Why the "* 2"?  I guess there is no definition for BUF2PAGES.  Ugh.
> I propose adding one, to avoid magic numbers.
> 
That what I tried to explain in the comment above, with off2=1 a string with
size 63 will be placed at s2+1 and the \0 will be at buf2 + page_size + 
1, thus
writing in the PROT_NONE protected page and breaking the test even for
simple_strncpy/stupid_strncpy.
With maxoffset*2 we have enough room on buf2 for when off1 and off2 are
maxoffset-1.

>> +
>> +  MEMSET (s1, 'a', maxoffset - 1);
>> +  s1[maxoffset - 1] = '\0';
>> +
>> +  /* Both strings are bounded to a page with write/read access and the next
> 
> nit, and totally subjective, but since I'm here, it's more common to use "read/write".
> 
>> +     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 offset up to maxoffset
> 
> nit: s/offset/offsets/
> 
>> +     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 +352,7 @@ test_main (void)
>>       }
>>
>>     do_random_tests ();
>> +  do_page_tests ();
>>     return ret;
>>   }
> 
> LGTM with the above addressed.
> 
> PC
> 

Thanks,

On 10/07/2020 10:56, Lucas A. M. Magalhaes wrote:
> Hi Raphael,
> Thanks fo the patch.
> 
> Quoting Raphael Moreira Zinsly (2020-07-08 17:20:09)
>> Changes since v5:
>> string/test-strncpy.c:
>>          - Adjusted s1 and s2 placements.
>>          - Used the same pattern as test-strncasecmp.
>>
>> --- >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 | 44 +++++++++++++++++++++++++++++++++++++++
>>   string/test-strncpy.c     | 36 ++++++++++++++++++++++++++++++++
>>   2 files changed, 80 insertions(+)
>>
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
>> index 6a9c27beae..628135b962 100644
>> --- a/string/test-strncasecmp.c
>> +++ b/string/test-strncasecmp.c
>> @@ -137,6 +137,49 @@ 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;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  const size_t maxoffset = 64;
>> +
>> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
>> +  memset (s1, 'a', maxoffset - 1);
>> +  s1[maxoffset - 1] = '\0';
> 
> OK.
>> +
>> +  s2 = (char *) buf2 + page_size - maxoffset;
>> +  memset (s2, 'a', maxoffset - 1);
>> +  s2[maxoffset - 1] = '\0';
>> +
> OK. buf2 is only 1 page size.
> 
>> +  /* At this point s1 and s2 points to distinct memory regions containing
>> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
>> +     page with write/read 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 offset 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);
>> +       }
>> +    }
>> +}
>> +
> 
> OK.
>>   static void
>>   do_random_tests (void)
>>   {
>> @@ -334,6 +377,7 @@ test_locale (const char *locale)
>>       }
>>   
>>     do_random_tests ();
>> +  do_page_tests ();
>>   }
>>   
> 
> OK.
>>   int
>> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
>> index c978753ad8..c8d4ad58a6 100644
>> --- a/string/test-strncpy.c
>> +++ b/string/test-strncpy.c
>> @@ -155,6 +155,41 @@ 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;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  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 + 1) * 2;
>> +
> 
> IDK if was the intention. If you add maxoffset to s2 it will still leave
> maxoffset + 2 bytes for s2 and not maxoffset as the comment says.
> 
That's right, leaving the + 1 was a mistake. Thanks for catching it!

>> +  MEMSET (s1, 'a', maxoffset - 1);
>> +  s1[maxoffset - 1] = '\0';
>> +
> 
> OK.
> 
>> +  /* Both strings are bounded to a page with write/read 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 offset 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);
> 
> maxoffset + 2?
>> +       }
>> +    }
>> +}
>> +
>>   static void
>>   do_random_tests (void)
>>   {
>> @@ -317,6 +352,7 @@ test_main (void)
>>       }
>>   
>>     do_random_tests ();
>> +  do_page_tests ();
>>     return ret;
>>   }
>>   
> 
> OK.
>> -- 
>> 2.26.2
>>

Best Regards,