This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH]: Use RAX_LP/RDX_LP on SAVE_PTR in sysdeps/x86_64/strtok.S


On Wed, Jun 13, 2012 at 1:09 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> This test is very sensitive to stack layout. ?-O1 with GCC 4.6 will
>> create a stack layout such that the test fails on x32 with a broken
>> strtok_r implementation.
>
> That is so fragile that I don't think this is a good thing to rely on in
> the test suite. ?Perhaps it would be better to write an explicit wrapper
> function in assembly that ensures the stack situation that tickles the bug.
>

>> +/* Test strtok_r
>
> It's worthwhile to say it's specifically a regression test for BZ #14229.
>
>> + ?return strcmp (tok, "udf");
>
> It's probably better to use != 0 so the failing return value is always 1
> even if strcmp returns some arbitrary positive or negative value.
>
>
> The fix itself looks fine to me now, as do most of the nits about the test.
> But I think we need a different method to ensure the test continues to test
> the problem scenario, that is less fragile than assuming the compiler
> happens to produce a certain stack layout.
>

How about this patch?

Thanks.


-- 
H.J.
---
	[BZ #14229]
	* string/Makefile (tests): Add tst-strtok_r.
	($(objpfx)tst-strtok_r): Depend on $(objpfx)tst-strtok_r_data.o.
	* string/tst-strtok_r.c: New file.
	* string/tst-strtok_r_data.c: Likewise.
	* sysdeps/x86_64/x32/tst-strtok_r_data.S: Likewise.
	* sysdeps/x86_64/strtok.S: Use RAX_LP/RDX_LP on SAVE_PTR.

diff --git a/string/Makefile b/string/Makefile
index 80923a2..f891cac 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -56,7 +56,7 @@ tests		:= tester inl-tester noinl-tester testcopy test-ffs	\
 		   tst-strtok tst-strxfrm bug-strcoll1 tst-strfry	\
 		   bug-strtok1 $(addprefix test-,$(strop-tests))	\
 		   bug-envz1 tst-strxfrm2 tst-endian tst-svc2		\
-		   bug-strstr1 bug-strchr1
+		   bug-strstr1 bug-strchr1 tst-strtok_r


 include ../Rules
@@ -75,6 +75,8 @@ CFLAGS-test-ffs.c = -fno-builtin
 CFLAGS-tst-inlcall.c = -fno-builtin
 CFLAGS-bug-strstr1.c = -fno-builtin

+$(objpfx)tst-strtok_r: $(objpfx)tst-strtok_r_data.o
+
 ifeq ($(cross-compiling),no)
 tests: $(objpfx)tst-svc.out
 $(objpfx)tst-svc.out: tst-svc.input $(objpfx)tst-svc
diff --git a/string/tst-strtok_r.c b/string/tst-strtok_r.c
new file mode 100644
index 0000000..2ec3bc4
--- /dev/null
+++ b/string/tst-strtok_r.c
@@ -0,0 +1,32 @@
+/* Test strtok_r regression for BZ #14229.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <string.h>
+
+extern char *saveptr;
+extern char line[];
+
+static int
+do_test (void)
+{
+  char *tok = strtok_r (line, " \t", &saveptr);
+  return strcmp (tok, "udf") != 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/string/tst-strtok_r_data.c b/string/tst-strtok_r_data.c
new file mode 100644
index 0000000..27f5369
--- /dev/null
+++ b/string/tst-strtok_r_data.c
@@ -0,0 +1,20 @@
+/* Test strtok_r regression for BZ #14229.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+char *saveptr;
+char line[] = "udf 75868 1 - Live 0xffffffffa0bfb000\n";
diff --git a/sysdeps/x86_64/strtok.S b/sysdeps/x86_64/strtok.S
index 150f4d6..3ed6521 100644
--- a/sysdeps/x86_64/strtok.S
+++ b/sysdeps/x86_64/strtok.S
@@ -1,6 +1,6 @@
 /* strtok (str, delim) -- Return next DELIM separated token from STR.
    For AMD x86-64.
-   Copyright (C) 1998,2000-2003,2005,2006 Free Software Foundation, Inc.
+   Copyright (C) 1998-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Based on i686 version contributed by Ulrich Drepper
    <drepper@cygnus.com>, 1998.
@@ -79,13 +79,12 @@ ENTRY (BP_SYM (FUNCTION))

 #ifdef USE_AS_STRTOK_R
 	/* The value is stored in the third argument.  */
-	movq %rdx, %rax
-	movq %rdx, %r9		/* Save value - see def. of SAVE_PTR.  */
-	movq (%rax), %rax
+	mov %RDX_LP, %R9_LP	/* Save value - see def. of SAVE_PTR.  */
+	mov (%rdx), %RAX_LP
 #else
 	/* The value is in the local variable defined above.  But
 	   we have to take care for PIC code.  */
-	movq SAVE_PTR, %rax
+	mov SAVE_PTR, %RAX_LP
 #endif
 	movq %r8, %rdx		/* Get start of string.  */

@@ -194,7 +193,7 @@ L(8):	cmpq %rax, %rdx
 	cmovne %rcx, %rdx

 	/* Store the pointer to the next character.  */
-	movq %rdx, SAVE_PTR
+	mov %RDX_LP, SAVE_PTR

 L(epilogue):
 	/* Remove the stopset table.  */
@@ -205,7 +204,7 @@ L(epilogue):
 L(returnNULL):
 	xorl %eax, %eax
 	/* Store the pointer to the next character.  */
-	movq %rdx, SAVE_PTR
+	mov %RDX_LP, SAVE_PTR
 	jmp L(epilogue)

 END (BP_SYM (FUNCTION))
diff --git a/sysdeps/x86_64/x32/tst-strtok_r_data.S
b/sysdeps/x86_64/x32/tst-strtok_r_data.S
new file mode 100644
index 0000000..cfbdb51
--- /dev/null
+++ b/sysdeps/x86_64/x32/tst-strtok_r_data.S
@@ -0,0 +1,28 @@
+/* Test strtok_r regression for BZ #14229.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Make sure that LINE is right after SAVEPTR so that updating SAVEPTR
+   with 8 byte store will override first 4 bytes in LINE.  */
+	.data
+	.globl	line
+	.globl	saveptr
+	.p2align 2
+saveptr:
+	.long	0
+line:
+	.string	"udf 75868 1 - Live 0xffffffffa0bfb000\n"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]