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

[Raoni Fassina Firmino]

Hi Raphael,


Following is my limited review, I may be misunderstanding something
crucial about the tests, so take my review with a grain of salt.

I understand that right now the test should pass, right? Is there any
patch to strncasecmp or strncpy to make it buggy to trigger the test, so
it catch the intended failure state it is checking?

For my education, the problem with the page boundaries can happen at the
beginning of the string or only at the end?

> 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';
> +
> +  s2 = (char *) buf2 + page_size - maxoffset;
> +  memset (s2, 'a', maxoffset - 1);
> +  s2[maxoffset - 1] = '\0';
> +

This is just a cosmetic suggestion: you can declare s* and set them like
do_random_test (But I am not sure if this is the standard or the
exception).


> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..05c56e06a5 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,37 @@ 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 last_offset = 64;
> +
> +  s2 = (CHAR *) buf2;
> +  s1 = (CHAR *) buf1;

I believe s* should be at the end of buf*, the same as in
test-strncasecmp.c?

Cosmetics suggestion: Same as in test-strncasecmp.c.


> +  MEMSET (s1, '\1', last_offset);
> +  s1[last_offset] = '\0';

Not sure if relevant, but in test-strncasecmp.c, the size is 63 + '\n',
here it is 64  + '\0'.  But My math may be off, so sorry in advance.


> +
> +  /* 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 last_offset
> +     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 < last_offset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < last_offset; off2++)
> +	{
> +	  FOR_EACH_IMPL (impl, 0)
> +	    do_one_test (impl, (CHAR *) (s2 + off2), (CHAR *) (s1 + off1),
> +			  last_offset - off1, last_offset + 1);

Should s2 be cleanup before each test? I am thinking that the copy my
fail but do_one_test compare the result as copied because the leftover
from the previous copy.

Cosmetic suggestion: Are the castings needed here as s* seems to be the
right type already.  I didn't test it without it though, so I am
genuinely asking.


o/
Raoni Fassina Firmino