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

Changes since v7:
	- Fixed comments to make them clearer.

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

Hi Raphael, think this is ready to go.
The tests work fine on ppc64le.
Thanks.

---
Lucas A. M. Magalhães

Hi Raphael,

Sorry for my late review, hope it is useful.

I Tested the patch on powerpc-linux-gnu (with kernel ppc64) on top of
master (0ad926f349) with no problems.


> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c

OK.


> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
...

> +  const size_t maxoffset = 64;

I am confused about the role of maxoffset == 64 and sizeof(CHAR), if the
intended value of 64 is for the buffer size in bytes or array elements.
But in any case I think It surely is not a problem to be larger than the
minimum 64 bytes, I am just pointing it out because I may be missing
something.


> +  /* Put s1 at the maxoffset from the edge of buf1's last page.  */
> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;

I don't understand how the position in the buffer is being calculated
here, IIUIC you want the last `maxoffset * sizeof(CHAR)` bytes at the
end of buf1, in this case `page_size / sizeof(CHAR)` doesn't make sense
for me, but my math may be off.


> +  /* s2 needs room to put a string with size of maxoffset + 1 at s2 +
> +     (maxoffset - 1).  */
> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;

Same as before, but also `maxoffset * 2` guarantees that only one of the
tests will touch the edge of buf2 page, I am not sure if this is the
intended case.


> +	    do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,
> +			 maxoffset + 1);

Reading do_one_test, it seems like len, here as `maxoffset - off1 - 1`,
should be `maxoffset - off1`, as it is supposed to be the size of src.
I know that because the trailing '\0' on src, this in practice will not
be a problem, but for a case when sizeof(src) and sizeof(dest) is the
same, do_one_test will ended up doing the last bit for len < n.

nit, and totally optional, you don't need "()" around (s2 + off2) and
(s1 + off1), I am only mentioning because you didn't use it for the
other parameters.



o/
Raoni

On 7/29/20 2:27 PM, Raphael Moreira Zinsly via Libc-alpha wrote:
> Changes since v7:
> 	- Fixed comments to make them clearer.

We don't need these for glibc 2.32, so I'm expecting you'll hold off
for 2.33 to open to commit this when the review is done.

Hi Raoni,
Thanks for the review

On 31/07/2020 11:14, Raoni Fassina Firmino wrote:
> Hi Raphael,
> 
> Sorry for my late review, hope it is useful.
> 
> I Tested the patch on powerpc-linux-gnu (with kernel ppc64) on top of
> master (0ad926f349) with no problems.
> 
> 
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> 
> OK.
> 
> 
>> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> ...
> 
>> +  const size_t maxoffset = 64;
> 
> I am confused about the role of maxoffset == 64 and sizeof(CHAR), if the
> intended value of 64 is for the buffer size in bytes or array elements.
> But in any case I think It surely is not a problem to be larger than the
> minimum 64 bytes, I am just pointing it out because I may be missing
> something.
> 
maxoffset is based on the usual register sizes as suggested here: 
https://sourceware.org/pipermail/libc-alpha/2020-June/114978.html

> 
>> +  /* Put s1 at the maxoffset from the edge of buf1's last page.  */
>> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
> 
> I don't understand how the position in the buffer is being calculated
> here, IIUIC you want the last `maxoffset * sizeof(CHAR)` bytes at the
> end of buf1, in this case `page_size / sizeof(CHAR)` doesn't make sense
> for me, but my math may be off.
> 
This was intended for test-wcsncpy, as `wchar_t` is bigger than `char` 
we need more space in the buffer to use the same test. Your suggestion 
makes sense, but the wmemset fails if s1 is placed with just `maxoffset 
* sizeof(CHAR)`, seems it is accessing the protected area.
The page test at test-strnlen also uses `page_size / sizeof(CHAR)`.

> 
>> +  /* s2 needs room to put a string with size of maxoffset + 1 at s2 +
>> +     (maxoffset - 1).  */
>> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;
> 
> Same as before, but also `maxoffset * 2` guarantees that only one of the
> tests will touch the edge of buf2 page, I am not sure if this is the
> intended case.
> 
What do you mean by one of the tests?
If you are referring to test-strncasecmp it's a different behavior, 
strncpy fills the `dest` string with NULL if the `src` is smaller than 
`N`, so it will reach the maxoffset+1 and the end of buf2.

> 
>> +	    do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,
>> +			 maxoffset + 1);
> 
> Reading do_one_test, it seems like len, here as `maxoffset - off1 - 1`,
> should be `maxoffset - off1`, as it is supposed to be the size of src.
> I know that because the trailing '\0' on src, this in practice will not
> be a problem, but for a case when sizeof(src) and sizeof(dest) is the
> same, do_one_test will ended up doing the last bit for len < n.
> 
The len expected on do_one_test is the number of chars in `src`, if you 
take a look at test-stpncpy.c:
#define STRNCPY_RESULT(dst, len, n) ((dst) + ((len) > (n) ? (n) : (len)))
Counting the trailing '\0' will use it to compare with stpncpy's result 
thus getting a false fail.

> nit, and totally optional, you don't need "()" around (s2 + off2) and
> (s1 + off1), I am only mentioning because you didn't use it for the
> other parameters.
> 
> 
> 
> o/
> Raoni
> 

Best Regards,

Hi Raphael,

Sorry that took so long for me to reply. It is clear now some of my
undertanding. still I don't grasp everything, but that is on me, I need
to learn a bit more to get all the details.

So for what I could grasp, It LGTM.

o/
Raoni

On 7/29/20 1:27 PM, Raphael Moreira Zinsly via Libc-alpha wrote:
> Changes since v7:
> 	- Fixed comments to make them clearer.
> 
> --- >8 ---
> 
> Adds tests to check if strings placed at page bondaries are

Minor grammar/spelling nit: "Add tests" and "page boundaries".

Similarly, commit title might be improved by rewording to something 
like: "string: test strncasecmp and strncpy near page boundaries"

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

This test looks OK.

>   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 maxoffset from the edge of buf1's last page.  */
> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
> +  /* s2 needs room to put a string with size of maxoffset + 1 at s2 +
> +     (maxoffset - 1).  */

Does this only test the interesting page boundary case of s2 when off2 
== (maxoffset-1)?  Should the n of do_one_test instead be maxoffset + 
(maxoffset - off2) to ensure each iteration straddles the guard page of 
s1 and s2?

> +  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);
> +	}
> +    }
> +}