Message ID | 20200715131631.7528-1-rzinsly@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v7] string: Adds tests for test-strncasecmp and test-strncpy | expand |
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 >
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,
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
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; }