wcsstr is broken

wcsstr is broken

[Szabolcs Nagy]

naive_wcsstr is wrong, h can overrun in the inner loop
as reported in irc:

16:36 < dkl> with certain input, it appears to be overrunning
16:36 < dkl> for example: wcsstr(L"ab", L"ac");
...
16:41 < dkl> in this case the inner one keeps increasing the "h" pointer past the null terminator
16:44 < dkl> I had a go at a patch too: http://jafile.com/uploads/dkl/0001-wcsstr-fix-buffer-overrun.patch

see patch below

>From 2b635da731496e037d1b18118d874acbad5f351a Mon Sep 17 00:00:00 2001
>From: dkl <daniel.c.klauer@web.de>
Date: Fri, 10 Aug 2012 05:32:44 +0200
Subject: [PATCH] wcsstr(): fix buffer overrun

A wcsstr() call such as:
        wcsstr(L"ab", L"ac");
causes naive_wcsstr() to be used. For this input however, it would overrun
the first input string, due to the inner loop continuing to increase h
past the null terminator.

The same problem existed in the naive_strstr() function for strstr(),
which however was commented out and thus not used anyways.
---
 src/string/strstr.c |   10 ----------
 src/string/wcsstr.c |   16 +++++++++++++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/string/strstr.c b/src/string/strstr.c
index 683cdd0..0649174 100644
--- a/src/string/strstr.c
+++ b/src/string/strstr.c
@@ -25,16 +25,6 @@ static char *fourbyte_strstr(const unsigned char *h, const unsigned char *n)
        return *h ? (char *)h-3 : 0;
 }

-#if 0
-static char *naive_strstr(const char *h, const char *n)
-{
-       size_t i;
-       for (i=0; n[i] && h[i]; i++)
-       for (   ; n[i] != h[i]; h++, i=0);
-       return n[i] ? 0 : (char *)h;
-}
-#endif
-
 #define MAX(a,b) ((a)>(b)?(a):(b))
 #define MIN(a,b) ((a)<(b)?(a):(b))

diff --git a/src/string/wcsstr.c b/src/string/wcsstr.c
index 966174f..6196a02 100644
--- a/src/string/wcsstr.c
+++ b/src/string/wcsstr.c
@@ -6,9 +6,19 @@
 static wchar_t *naive_wcsstr(const wchar_t *h, const wchar_t *n)
 {
        size_t i;
-       for (i=0; n[i] && h[i]; i++)
-       for (   ; n[i] != h[i]; h++, i=0);
-       return n[i] ? 0 : (wchar_t *)h;
+
+       for ( ; *h; h++) {
+               /* check for needle starting from here */
+               i = 0;
+               while (1) {
+                       if (!n[i]) return (wchar_t *)h; /* matched whole n */
+                       if (!h[i]) return 0; /* end of h, can't match anymore */
+                       if (h[i] != n[i]) break;
+                       i++;
+               }
+       }
+
+       return 0;
 }

 #define MAX(a,b) ((a)>(b)?(a):(b))
--
1.7.9.5

Re: wcsstr is broken

[Rich Felker]

On Fri, Aug 10, 2012 at 05:41:36PM +0200, Szabolcs Nagy wrote:
> naive_wcsstr is wrong, h can overrun in the inner loop
> as reported in irc:
> 
> 16:36 < dkl> with certain input, it appears to be overrunning
> 16:36 < dkl> for example: wcsstr(L"ab", L"ac");
> ....
> 16:41 < dkl> in this case the inner one keeps increasing the "h" pointer past the null terminator
> 16:44 < dkl> I had a go at a patch too: http://jafile.com/uploads/dkl/0001-wcsstr-fix-buffer-overrun.patch
> 
> see patch below

Is my fix in git acceptable (just removing the buggy function and
always calling twoway) or would you like to put back a working
function? Looking back, I find it kind of ugly to have a function
written to handle arbitrary-length needles that's only used for really
short ones...

Rich