[BZ,17506] fix tst-strcoll-overflow returning before timeout
diff mbox

Message ID 544B8E17.1020807@web.de
State New
Headers show

Commit Message

Leonhard Holz Oct. 25, 2014, 11:48 a.m. UTC
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.

diff --git a/string/tst-strcoll-overflow.c b/string/tst-strcoll-overflow.c
index e7a43ea..0d764af 100644
--- a/string/tst-strcoll-overflow.c
+++ b/string/tst-strcoll-overflow.c
@@ -57,5 +57,6 @@ do_test (void)
 
 #define TIMEOUT 300
 #define EXPECTED_SIGNAL SIGALRM
+#define EXPECTED_STATUS 0
 #define TEST_FUNCTION do_test ()
 #include "../test-skeleton.c"

Comments

Stefan Liebler Oct. 27, 2014, 8:41 a.m. UTC | #1
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.
>
Siddhesh Poyarekar Nov. 4, 2014, 5:48 p.m. UTC | #2
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
Leonhard Holz Nov. 6, 2014, 7:52 a.m. UTC | #3
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
>
Siddhesh Poyarekar Nov. 6, 2014, 9:10 a.m. UTC | #4
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
Leonhard Holz Nov. 6, 2014, 11:57 a.m. UTC | #5
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
Siddhesh Poyarekar Nov. 6, 2014, 3:07 p.m. UTC | #6
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
Leonhard Holz Nov. 6, 2014, 8:43 p.m. UTC | #7
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.
Leonhard Holz Nov. 12, 2014, 10:28 a.m. UTC | #8
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.
Siddhesh Poyarekar Nov. 12, 2014, 11:07 a.m. UTC | #9
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

Patch
diff mbox

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