Jim Meyering wrote:
> Jim Meyering wrote:
>
>> Ralf Wildenhues wrote:
>>
>>> [ this is http://thread.gmane.org/gmane.comp.sysutils.autoconf.bugs/7834
>>> from http://gcc.gnu.org/ml/gcc-patches/2011-02/msg01480.html
>>> adding bug-gnulib; followups can elide bug-autoconf ]
>>>
>>> * Ralf Wildenhues wrote on Thu, Feb 24, 2011 at 07:24:35AM CET:
>>>> IOW, it looks like the replacement code in strstr.c and str-two-way.h
>>>> has a bug (or glibc strchr, which seems rather unlikely). Besides
>>>> copyright year bumps, these two files have not been updated since in
>>>> gnulib.
>>>
>>> Here's a reproducer. Link with gnulib's strstr.c and it will fail.
>>
>> Wow. Thanks for that, guys.
>>
>> Here's a fix that passes this test, which includes Ralf's test case:
>>
>> ./gnulib-tool --create-testdir --with-tests --test strstr
>>
>> I haven't finished testing, and will add a comment attributing
>> the test case. Oh, and I'll make the test assertion stronger
>> (to include the precise offset).
>
> FYI, this patch also corrects the identical period = ...
> assignment in two_way_long_needle, but I haven't yet added a test
> to exercise that case. That's next.
Constructing a test for the latter case was harder, so I made the
test brute force it. That gives better coverage besides. The failed
assertion is triggered with offset 52 when using the original period
calculation in two_way_long_needle.
Here's what I've just pushed:
>From 98d62d70dc39859781c830aaf1b81a97a576d99c Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Thu, 24 Feb 2011 10:57:22 +0100
Subject: [PATCH] strstr: fix a bug whereby strstr would mistakenly return NULL
* lib/str-two-way.h (two_way_short_needle): Correct off-by-one error
in period calculation.
(two_way_long_needle): Likewise.
Reported by Ralf Wildenhues, with the short needle and haystack.
* tests/test-strstr.c: Add Ralf's test case to trigger the bug.
Add a more involved test to trigger the bug in two_way_long_needle.
---
ChangeLog | 10 ++++++
lib/str-two-way.h | 4 +-
tests/test-strstr.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 91 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 139b38f..7795cce 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2011-02-24 Jim Meyering <meyer...@redhat.com>
+
+ strstr: fix a bug whereby strstr would mistakenly return NULL
+ * lib/str-two-way.h (two_way_short_needle): Correct off-by-one error
+ in period calculation.
+ (two_way_long_needle): Likewise.
+ Reported by Ralf Wildenhues, with the short needle and haystack.
+ * tests/test-strstr.c: Add Ralf's test case to trigger the bug.
+ Add a more involved test to trigger the bug in two_way_long_needle.
+
2011-02-24 Stefano Lattarini <stefano.lattar...@gmail.com> (tiny change)
gnulib-tool: remove use of bold display in help screen
diff --git a/lib/str-two-way.h b/lib/str-two-way.h
index dd80976..317612c 100644
--- a/lib/str-two-way.h
+++ b/lib/str-two-way.h
@@ -284,7 +284,7 @@ two_way_short_needle (const unsigned char *haystack, size_t
haystack_len,
{
/* The two halves of needle are distinct; no extra memory is
required, and any mismatch results in a maximal shift. */
- period = MAX (suffix, needle_len - suffix) + 1;
+ period = MAX (suffix, needle_len - suffix);
j = 0;
while (AVAILABLE (haystack, haystack_len, j, needle_len))
{
@@ -407,7 +407,7 @@ two_way_long_needle (const unsigned char *haystack, size_t
haystack_len,
/* The two halves of needle are distinct; no extra memory is
required, and any mismatch results in a maximal shift. */
size_t shift;
- period = MAX (suffix, needle_len - suffix) + 1;
+ period = MAX (suffix, needle_len - suffix);
j = 0;
while (AVAILABLE (haystack, haystack_len, j, needle_len))
{
diff --git a/tests/test-strstr.c b/tests/test-strstr.c
index f63cb33..718ead7 100644
--- a/tests/test-strstr.c
+++ b/tests/test-strstr.c
@@ -184,5 +184,84 @@ main (int argc, char *argv[])
/* Sublinear speed is only possible in memmem; strstr must examine
every character of haystack to find its length. */
+
+ {
+ /* Ensure that with a barely periodic "short" needle, strstr's
+ search does not mistakenly skip just past the match point.
+ This use of strstr would mistakenly return NULL before
+ gnulib v0.0-4927. */
+ const char *haystack =
+ "\n"
+ "with_build_libsubdir\n"
+ "with_local_prefix\n"
+ "with_gxx_include_dir\n"
+ "with_cpp_install_dir\n"
+ "enable_generated_files_in_srcdir\n"
+ "with_gnu_ld\n"
+ "with_ld\n"
+ "with_demangler_in_ld\n"
+ "with_gnu_as\n"
+ "with_as\n"
+ "enable_largefile\n"
+ "enable_werror_always\n"
+ "enable_checking\n"
+ "enable_coverage\n"
+ "enable_gather_detailed_mem_stats\n"
+ "enable_build_with_cxx\n"
+ "with_stabs\n"
+ "enable_multilib\n"
+ "enable___cxa_atexit\n"
+ "enable_decimal_float\n"
+ "enable_fixed_point\n"
+ "enable_threads\n"
+ "enable_tls\n"
+ "enable_objc_gc\n"
+ "with_dwarf2\n"
+ "enable_shared\n"
+ "with_build_sysroot\n"
+ "with_sysroot\n"
+ "with_specs\n"
+ "with_pkgversion\n"
+ "with_bugurl\n"
+ "enable_languages\n"
+ "with_multilib_list\n";
+ const char *needle = "\n"
+ "with_gnu_ld\n";
+ const char* p = strstr (haystack, needle);
+ ASSERT (p - haystack == 114);
+ }
+
+ {
+ /* Like the above, but trigger the flaw in two_way_long_needle
+ by using a needle of length LONG_NEEDLE_THRESHOLD (32) or greater.
+ Rather than trying to find the right alignment manually, I've
+ arbitrarily chosen the following needle and template for the
+ haystack, and ensure that for each placement of the needle in
+ that haystack, strstr finds it. */
+ const char *needle = "\nwith_gnu_ld-extend-to-len-32-b\n";
+ const char *h =
+ "\n"
+ "with_build_libsubdir\n"
+ "with_local_prefix\n"
+ "with_gxx_include_dir\n"
+ "with_cpp_install_dir\n"
+ "with_e_\n"
+ "..............................\n"
+ "with_FGHIJKLMNOPQRSTUVWXYZ\n"
+ "with_567890123456789\n" "with_multilib_list\n";
+ size_t h_len = strlen (h);
+ char *haystack = malloc (h_len + 1);
+ ASSERT (haystack);
+ size_t i;
+ for (i = 0; i < h_len - strlen (needle); i++)
+ {
+ memcpy (haystack, h, h_len + 1);
+ memcpy (haystack + i, needle, strlen (needle) + 1);
+ const char *p = strstr (haystack, needle);
+ ASSERT (p);
+ ASSERT (p - haystack == i);
+ }
+ }
+
return 0;
}
--
1.7.4.1.16.g759e8
