| Message ID | 544B8E17.1020807@web.de |
|---|---|
| State | New |
| Headers | show |
Hi Leonhard, this fixes bug 17506 on s390, ppc, x86_64. Thanks On 10/25/2014 01:48 PM, Leonhard Holz wrote: > Modifies the test examination in test-skeleton.c so that a test can be > successful if it is interrupted or it returns uninterrupted with the > expected status. For this both EXPECTED_SIGNAL and EXPECTED_STATUS have > to be set, as is done in tst-strcoll-overflow.c. > > 2014-10-25 Leonhard Holz <leonhard.holz@web.de> > > * test-skeleton.c (main): Return successful if one of > EXPECTED_SIGNAL or EXPECTED_STATUS is met when both given. > * string/tst-strcoll-overflow.c: Define expected status. >
On Sat, Oct 25, 2014 at 01:48:39PM +0200, Leonhard Holz wrote: > Modifies the test examination in test-skeleton.c so that a test can be > successful if it is interrupted or it returns uninterrupted with the > expected status. For this both EXPECTED_SIGNAL and EXPECTED_STATUS have to > be set, as is done in tst-strcoll-overflow.c. This is unnecessary. The test is intended to catch crashes on any kind of overflows that may have been caused in strcoll due to large string inputs (CVE-2012-4412 for example). The timeout and alarm are there because the non-cached version would go on forever since it was really slow. With your performance improvements the test completes sucessfully without a crash but it doesn't actually go through all four passes. The ideal fix to the test case would be to have a really large string that is not binary identical like it currently is, but that'll be quite hard to do right now. For now, it should be sufficient to just get rid of the EXPECTED_SIGNAL, reduce the TIMEOUT to a smaller value and adjust the big comment in the test to reflect the new reality. Siddhesh
Well, the former implementation just relied on running into timeout because of swapping and collating time, but it could not guarantee it. With enough hardware resources (maybe 10GB memory and a high end cpu) the test would have completeted within the five minutes boundary - and then counted as failed. Now the needed hardware resources to complete in time have been lowered a lot but the overall behaviour has not changed: If the test machine is capable enough the test does complete, if not it runs into timeout (there is still an allocation of 1GB). Because of this I decided to fix the test implementation to support both outcomes. If this feels to luxurious I'd vote for setting the timeout much lower (30s?) so that the test suite completes faster. Best, Leonhard Am 04.11.2014 18:48, schrieb Siddhesh Poyarekar: > On Sat, Oct 25, 2014 at 01:48:39PM +0200, Leonhard Holz wrote: >> Modifies the test examination in test-skeleton.c so that a test can be >> successful if it is interrupted or it returns uninterrupted with the >> expected status. For this both EXPECTED_SIGNAL and EXPECTED_STATUS have to >> be set, as is done in tst-strcoll-overflow.c. > > This is unnecessary. The test is intended to catch crashes on any > kind of overflows that may have been caused in strcoll due to large > string inputs (CVE-2012-4412 for example). The timeout and alarm are > there because the non-cached version would go on forever since it was > really slow. > > With your performance improvements the test completes sucessfully > without a crash but it doesn't actually go through all four passes. > The ideal fix to the test case would be to have a really large string > that is not binary identical like it currently is, but that'll be > quite hard to do right now. For now, it should be sufficient to just > get rid of the EXPECTED_SIGNAL, reduce the TIMEOUT to a smaller value > and adjust the big comment in the test to reflect the new reality. > > Siddhesh >
On Thu, Nov 06, 2014 at 08:52:16AM +0100, Leonhard Holz wrote: > Well, the former implementation just relied on running into timeout because > of swapping and collating time, but it could not guarantee it. With enough > hardware resources (maybe 10GB memory and a high end cpu) the test would > have completeted within the five minutes boundary - and then counted as > failed. This test should have succeeded on alarm as well as normal completion, but may not be the case for a lot of EXPECTED_SIGNAL tests where a normal completion is a failure and the test may not have handled it correctly, i.e. returning non-zero finally. If you can prove that all tests that use EXPECTED_SIGNAL handle completion correctly then your patch may be acceptable. > Now the needed hardware resources to complete in time have been lowered a > lot but the overall behaviour has not changed: If the test machine is > capable enough the test does complete, if not it runs into timeout (there is > still an allocation of 1GB). Because of this I decided to fix the test > implementation to support both outcomes. If this feels to luxurious I'd vote > for setting the timeout much lower (30s?) so that the test suite completes > faster. Set the timeout to a value that is large enough to allow the test to complete without an alarm on commodity hardware. If someone finds that it is not large enough they can always patch it to increase it. Siddhesh
Am 06.11.2014 10:10, schrieb Siddhesh Poyarekar: > On Thu, Nov 06, 2014 at 08:52:16AM +0100, Leonhard Holz wrote: >> Well, the former implementation just relied on running into timeout because >> of swapping and collating time, but it could not guarantee it. With enough >> hardware resources (maybe 10GB memory and a high end cpu) the test would >> have completeted within the five minutes boundary - and then counted as >> failed. > > This test should have succeeded on alarm as well as normal completion, > but may not be the case for a lot of EXPECTED_SIGNAL tests where a > normal completion is a failure and the test may not have handled it > correctly, i.e. returning non-zero finally. If you can prove that all > tests that use EXPECTED_SIGNAL handle completion correctly then your > patch may be acceptable. > A "grep EXPECTED_SIGNAL * -R" reveals that all other tests that define EXPECTED_SIGNAL are in nptl and a "grep EXPECTED_SIGNAL * -R -l | xargs grep EXPECTED_STATUS" there shows that none of them defines EXPECTED_STATUS. So all theses test end up in line 394 of the patched test-skeleton.c if they return normaly which is: printf ("Expected signal '%s' from child, got none\n", strsignal (EXPECTED_SIGNAL)); exit (1); Leonhard
On Thu, Nov 06, 2014 at 12:57:52PM +0100, Leonhard Holz wrote: > A "grep EXPECTED_SIGNAL * -R" reveals that all other tests that define > EXPECTED_SIGNAL are in nptl and a "grep EXPECTED_SIGNAL * -R -l | xargs grep > EXPECTED_STATUS" there shows that none of them defines EXPECTED_STATUS. So > all theses test end up in line 394 of the patched test-skeleton.c if they > return normaly which is: > > printf ("Expected signal '%s' from child, got none\n", > strsignal (EXPECTED_SIGNAL)); > exit (1); Fair enough. I'll commit this once you confirm that 'make xcheck' does not introduce any new failures in the testsuite. Siddhesh
Am 06.11.2014 16:07, schrieb Siddhesh Poyarekar: > On Thu, Nov 06, 2014 at 12:57:52PM +0100, Leonhard Holz wrote: >> A "grep EXPECTED_SIGNAL * -R" reveals that all other tests that define >> EXPECTED_SIGNAL are in nptl and a "grep EXPECTED_SIGNAL * -R -l | xargs grep >> EXPECTED_STATUS" there shows that none of them defines EXPECTED_STATUS. So >> all theses test end up in line 394 of the patched test-skeleton.c if they >> return normaly which is: >> >> printf ("Expected signal '%s' from child, got none\n", >> strsignal (EXPECTED_SIGNAL)); >> exit (1); > > Fair enough. I'll commit this once you confirm that 'make xcheck' > does not introduce any new failures in the testsuite. > I confirm that.
Any news here? Maybe I was a bit brief, what I wanted to say is that I did a make tests and make xcheck before and after the changes and there was no change in test results. Am 06.11.2014 21:43, schrieb Leonhard Holz: > Am 06.11.2014 16:07, schrieb Siddhesh Poyarekar: >> On Thu, Nov 06, 2014 at 12:57:52PM +0100, Leonhard Holz wrote: >>> A "grep EXPECTED_SIGNAL * -R" reveals that all other tests that define >>> EXPECTED_SIGNAL are in nptl and a "grep EXPECTED_SIGNAL * -R -l | >>> xargs grep >>> EXPECTED_STATUS" there shows that none of them defines >>> EXPECTED_STATUS. So >>> all theses test end up in line 394 of the patched test-skeleton.c if >>> they >>> return normaly which is: >>> >>> printf ("Expected signal '%s' from child, got none\n", >>> strsignal (EXPECTED_SIGNAL)); >>> exit (1); >> >> Fair enough. I'll commit this once you confirm that 'make xcheck' >> does not introduce any new failures in the testsuite. >> > > I confirm that.
On Wed, Nov 12, 2014 at 11:28:22AM +0100, Leonhard Holz wrote: > Any news here? Maybe I was a bit brief, what I wanted to say is that I did a > make tests and make xcheck before and after the changes and there was no > change in test results. Sorry, I forgot to commit this last week. I'll do it today. Siddhesh
diff --git a/test-skeleton.c b/test-skeleton.c index c1278ca..3621009 100644 --- a/test-skeleton.c +++ b/test-skeleton.c @@ -383,39 +383,46 @@ main (int argc, char *argv[]) exit (1); } -#ifndef EXPECTED_SIGNAL - /* We don't expect any signal. */ -# define EXPECTED_SIGNAL 0 -#endif - if (WTERMSIG (status) != EXPECTED_SIGNAL) + /* Process terminated normaly without timeout etc. */ + if (WIFEXITED (status)) { - if (EXPECTED_SIGNAL != 0) - { - if (WTERMSIG (status) == 0) - printf ("Expected signal '%s' from child, got none\n", - strsignal (EXPECTED_SIGNAL)); - else - printf ("Incorrect signal from child: got `%s', need `%s'\n", - strsignal (WTERMSIG (status)), - strsignal (EXPECTED_SIGNAL)); - } - else - printf ("Didn't expect signal from child: got `%s'\n", - strsignal (WTERMSIG (status))); - exit (1); - } - - /* Simply exit with the return value of the test. */ #ifndef EXPECTED_STATUS - return WEXITSTATUS (status); +# ifndef EXPECTED_SIGNAL + /* Simply exit with the return value of the test. */ + return WEXITSTATUS (status); +# else + printf ("Expected signal '%s' from child, got none\n", + strsignal (EXPECTED_SIGNAL)); + exit (1); +# endif #else - if (WEXITSTATUS (status) != EXPECTED_STATUS) + if (WEXITSTATUS (status) != EXPECTED_STATUS) + { + printf ("Expected status %d, got %d\n", + EXPECTED_STATUS, WEXITSTATUS (status)); + exit (1); + } + + return 0; +#endif + } + /* Process was killed by timer or other signal. */ + else { - printf ("Expected status %d, got %d\n", - EXPECTED_STATUS, WEXITSTATUS (status)); +#ifndef EXPECTED_SIGNAL + printf ("Didn't expect signal from child: got `%s'\n", + strsignal (WTERMSIG (status))); exit (1); - } +#else + if (WTERMSIG (status) != EXPECTED_SIGNAL) + { + printf ("Incorrect signal from child: got `%s', need `%s'\n", + strsignal (WTERMSIG (status)), + strsignal (EXPECTED_SIGNAL)); + exit (1); + } - return 0; + return 0; #endif + } }