Bug 185 - length bug in stat_cache_lookup() in Samba 3.0.0beta1
length bug in stat_cache_lookup() in Samba 3.0.0beta1
Status: CLOSED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.0preX
All All
: P2 normal
: none
Assigned To: Jeremy Allison
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-06-22 02:27 UTC by TAKAHASHI Motonobu
Modified: 2005-08-24 10:18 UTC (History)
1 user (show)

See Also:


Attachments
Futher work on stat-cache and change-of-size (5.28 KB, patch)
2003-07-20 04:39 UTC, Andrew Bartlett
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description TAKAHASHI Motonobu 2003-06-22 02:27:27 UTC
I checked Samba 3.0.0beta1 and found a bug.

If we have a character-set whose length of upper character and of
lower character is different, the bug actually occurs.

/*
If 'A' were 1 byte length and 'a' were 2 byte length, the problem
would occur in English. In Japanese, there are some character-sets
that have different length lower and upper character.
*/

At statcache.c:250, stat_cache_lookup()

memcpy(name, scp->translated_path, MIN(sizeof(pstring-1),
  scp->translated_path_length));

If the length of "name" is different from scp->translated_path_length,
the problem will occur.
scp->translated_path_length is the length of strupper("name").
Comment 1 Andrew Bartlett 2003-06-22 18:04:23 UTC
Fun :-)

This is Samba 3.0's weakest point.  However, if the length is shorter *or*
longer, then we need to rework this entire function - as it is operating
in-place on thie existing name.

We really need to translate a path into a linked list of name components, or
something like that, so we can extend/truncate each one as required.
Comment 2 TAKAHASHI Motonobu 2003-06-24 09:55:29 UTC
Here is a patch for this problem written by MORIYAMA Masayuki.
He and I checked the patch.

Indeed this patch is not a refined one. If a name is not in the
cache, strrchr_m() is twice called.

--- Cut Here ---
diff -Nur ../../samba-3.0.0beta1.orig/source/smbd/statcache.c ./smbd/statcache.\
c
--- ../../samba-3.0.0beta1.orig/source/smbd/statcache.c 2003-06-08 02:57:39.000\
000000 +0900
+++ ./smbd/statcache.c  2003-06-23 00:56:45.000000000 +0900
@@ -193,6 +193,8 @@
 {
   stat_cache_entry *scp;
   pstring chk_name;
+  pstring notrans_name;
+  pstring last_component;
   size_t namelen;
   hash_element *hash_elem;
   char *sp;
@@ -215,6 +217,9 @@
   }

   pstrcpy(chk_name, name);
+  pstrcpy(notrans_name, name);
+  last_component[0] = '\0';
+
   if(!case_sensitive)
     strupper( chk_name );

@@ -227,6 +232,10 @@
       sp = strrchr_m(chk_name, '/');
       if (sp) {
         *sp = '\0';
+        sp = strrchr_m(notrans_name, '/');
+        *sp = '\0';
+        strncpy(last_component, &name[sp - notrans_name], sizeof(pstring));
+        last_component[sizeof(pstring)-1] = '\0';
       } else {
         /*
          * We reached the end of the name - no match.
@@ -247,7 +256,12 @@
         hash_remove(&stat_cache, hash_elem);
         return False;
       }
-      memcpy(name, scp->translated_path, MIN(sizeof(pstring)-1, scp->translate\
d_path_length));
+      strncpy(name, scp->translated_path, sizeof(pstring));
+      if (last_component[0] != '\0') {
+          strncpy(&name[scp->translated_path_length], last_component,
+                  sizeof(pstring) - scp->translated_path_length);
+      }
+      name[sizeof(pstring)-1] = '\0';

       /* set pointer for 'where to start' on fixing the rest of the name */
       *start = &name[scp->translated_path_length];
--- Cut Here ---
Comment 3 TAKAHASHI Motonobu 2003-06-24 09:56:35 UTC
MORIYAMA Masayuki's address is msyk _at_ mtg.biglobe.ne.jp
Comment 4 Jeremy Allison 2003-07-01 16:37:01 UTC
This is my code, I'll take this one.
Jeremy.
Comment 5 Jeremy Allison 2003-07-02 12:23:23 UTC
I'm not too happy with this patch due to the number of new pstrings
and string copies it adds into the mainline case (but thanks for the suggestion).
How about the following - I'm going to check this into SAMBA_3_0
and if you could test with the filename in question it I'd really
appreciate it.

Jeremy.

Index: lib/util_str.c
===================================================================
RCS file: /data/cvs/samba/source/lib/util_str.c,v
retrieving revision 1.47.2.26
diff -u -r1.47.2.26 util_str.c
--- lib/util_str.c	25 Jun 2003 17:41:04 -0000	1.47.2.26
+++ lib/util_str.c	2 Jul 2003 18:58:54 -0000
@@ -1111,6 +1111,26 @@
 	return (char *)(s+strlen(s2));
 }
 
+/***********************************************************************
+ Return the equivalent of doing strrchr 'n' times - always going
+ backwards.
+***********************************************************************/
+
+char *strnrchr_m(const char *s, char c, unsigned int n)
+{
+	wpstring ws;
+	pstring s2;
+	smb_ucs2_t *p;
+
+	push_ucs2(NULL, ws, s, sizeof(ws), STR_TERMINATE);
+	p = strnrchr_w(ws, UCS2_CHAR(c), n);
+	if (!p)
+		return NULL;
+	*p = 0;
+	pull_ucs2_pstring(s2, ws);
+	return (char *)(s+strlen(s2));
+}
+
 /**
  Convert a string to lower case.
 **/
Index: lib/util_unistr.c
===================================================================
RCS file: /data/cvs/samba/source/lib/util_unistr.c,v
retrieving revision 1.92.2.9
diff -u -r1.92.2.9 util_unistr.c
--- lib/util_unistr.c	23 Apr 2003 14:07:33 -0000	1.92.2.9
+++ lib/util_unistr.c	2 Jul 2003 18:58:54 -0000
@@ -391,8 +391,9 @@
 }
 
 /*******************************************************************
-wide strchr()
+ Wide strchr().
 ********************************************************************/
+
 smb_ucs2_t *strchr_w(const smb_ucs2_t *s, smb_ucs2_t c)
 {
 	while (*s != 0) {
@@ -409,6 +410,10 @@
 	return strchr_w(s, UCS2_CHAR(c));
 }
 
+/*******************************************************************
+ Wide strrchr().
+********************************************************************/
+
 smb_ucs2_t *strrchr_w(const smb_ucs2_t *s, smb_ucs2_t c)
 {
 	const smb_ucs2_t *p = s;
@@ -422,8 +427,30 @@
 }
 
 /*******************************************************************
-wide strstr()
+ Wide version of strrchr that returns after doing strrchr 'n' times.
+********************************************************************/
+
+smb_ucs2_t *strnrchr_w(const smb_ucs2_t *s, smb_ucs2_t c, unsigned int n)
+{
+	const smb_ucs2_t *p = s;
+	int len = strlen_w(s);
+	if (len == 0 || !n)
+		return NULL;
+	p += (len - 1);
+	do {
+		if (c == *p)
+			n--;
+
+		if (!n)
+			return (smb_ucs2_t *)p;
+	} while (p-- != s);
+	return NULL;
+}
+
+/*******************************************************************
+ Wide strstr().
 ********************************************************************/
+
 smb_ucs2_t *strstr_w(const smb_ucs2_t *s, const smb_ucs2_t *ins)
 {
 	smb_ucs2_t *r;
Index: smbd/statcache.c
===================================================================
RCS file: /data/cvs/samba/source/smbd/statcache.c,v
retrieving revision 1.13.2.6
diff -u -r1.13.2.6 statcache.c
--- smbd/statcache.c	2 Jul 2003 00:08:29 -0000	1.13.2.6
+++ smbd/statcache.c	2 Jul 2003 18:58:54 -0000
@@ -198,6 +198,8 @@
 	size_t namelen;
 	hash_element *hash_elem;
 	char *sp;
+	BOOL sizechanged = False;
+	unsigned int num_components = 0;
 
 	if (!lp_stat_cache())
 		return False;
@@ -217,8 +219,17 @@
 	}
 
 	pstrcpy(chk_name, name);
-	if(!case_sensitive)
+
+	if(!case_sensitive) {
 		strupper( chk_name );
+		/*
+		 * In some language encodings the length changes
+		 * if we uppercase. We need to treat this differently
+		 * below.
+		 */
+		if (strlen(chk_name) != namelen)
+			sizechanged = True;
+	}
 
 	while (1) {
 		hash_elem = hash_lookup(&stat_cache, chk_name);
@@ -229,6 +240,13 @@
 			sp = strrchr_m(chk_name, '/');
 			if (sp) {
 				*sp = '\0';
+				/*
+				 * Count the number of times we have done this,
+				 * we'll need it when reconstructing the string.
+				 */
+				if (sizechanged)
+					num_components++;
+
 			} else {
 				/*
 				 * We reached the end of the name - no match.
@@ -249,7 +267,20 @@
 				hash_remove(&stat_cache, hash_elem);
 				return False;
 			}
-			memcpy(name, scp->translated_path, MIN(sizeof(pstring)-1,
scp->translated_path_length));
+
+			if (!sizechanged) {
+				memcpy(name, scp->translated_path, MIN(sizeof(pstring)-1,
scp->translated_path_length));
+			} else {
+				pstring last_component;
+				sp = strnrchr_m(name, '/', num_components);
+				if (!sp) {
+					/* Logic error. */
+					smb_panic("logic error in stat_cache_lookup\n");
+				}
+				pstrcpy(last_component, sp);
+				pstrcpy(name, scp->translated_path);
+				pstrcat(name, last_component);
+			}
 
 			/* set pointer for 'where to start' on fixing the rest of the name */
 			*start = &name[scp->translated_path_length];
Comment 6 TAKAHASHI Motonobu 2003-07-05 09:39:22 UTC
This patch cause smbd crash 
when num_components = 0 && sizechanged = true.

Here is the patch written by MORIYAMA Masayuki <msyk@mtg.biglobe.ne.jp> .

--- cut here ---
--- smbd/statcache.c.orig       2003-07-03 18:57:20.000000000
+0900
+++ smbd/statcache.c    2003-07-03 18:58:14.000000000 +0900
@@ -270,6 +270,8 @@

                        if (!sizechanged) {
                                memcpy(name, scp->translated_path, MIN(sizeof(p\
string)-1, scp->translated_path_length));
+                       } else if (num_components == 0) {
+                               pstrcpy(name, scp->translated_path);
                        } else {
                                pstring last_component;
                                sp = strnrchr_m(name, '/', num_components);
--- cut here ---
Comment 7 Jeremy Allison 2003-07-07 13:22:56 UTC
Applied - thanks. I think this is fixed now !
Thanks,
Jeremy.
Comment 8 Andrew Bartlett 2003-07-20 04:39:21 UTC
Created attachment 53 [details]
Futher work on stat-cache and change-of-size

I'm wondering about the case where the string gets longer as part of the
conversion.  Does this patch help?

This should also not panic if the conversion fails, simply not caching it
instead.
Comment 9 Andrew Bartlett 2003-07-20 04:40:24 UTC
Re-opening this, as I think we can do a bit more work towards this.
Comment 10 Andrew Bartlett 2003-07-26 21:23:13 UTC
I've applied my patch, but I think there is still some work (and a lot of
testing) to be done in this area.
Comment 11 Gerald (Jerry) Carter 2003-08-02 11:45:09 UTC
Andrew, is the original bug fixed?  If so, then close this one out.
Comment 12 Andrew Bartlett 2003-08-02 18:20:36 UTC
Well, as I've applied the patch, and can't seem to raise any more
testing/comment here, I see no point leaving this bug open.

It's not perfect, but it's better than it was, and the case I have my biggest
concern about has a DEBUG(0,  on it.
Comment 13 Gerald (Jerry) Carter 2005-02-07 08:39:17 UTC
originally reported against 3.0.0beta1.  CLeaning out 
non-production release versions.
Comment 14 Gerald (Jerry) Carter 2005-08-24 10:18:28 UTC
sorry for the same, cleaning up the database to prevent unecessary reopens of bugs.