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").
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.
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 ---
MORIYAMA Masayuki's address is msyk _at_ mtg.biglobe.ne.jp
This is my code, I'll take this one. Jeremy.
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];
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 ---
Applied - thanks. I think this is fixed now ! Thanks, Jeremy.
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.
Re-opening this, as I think we can do a bit more work towards this.
I've applied my patch, but I think there is still some work (and a lot of testing) to be done in this area.
Andrew, is the original bug fixed? If so, then close this one out.
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.
originally reported against 3.0.0beta1. CLeaning out non-production release versions.
sorry for the same, cleaning up the database to prevent unecessary reopens of bugs.