The Samba-Bugzilla – Bug 7081
vfs_expand_msdfs doesn't work correctly (with fix identified)
Last modified: 2010-02-28 07:57:36 UTC
We've been using vfs_expand_msdfs for years with custom modifications. We recently updated from 3.0.x to 3.4.x and found the vfs_expand_msdfs didn't work. This is the error:
[2010/01/28 13:53:58, 0, pid=31373] lib/util_str.c:532(safe_strcpy_fn)
ERROR: string overflow by 1 (6 - 5) in safe_strcpy [msdfs:servername.washington.edu\homes]
This appears to be the same as bug 5998 reported a long while ago against Samba 3.2. I'd guess that expand_msdfs doesn't get much use since this bug has been around without much complaint.
The bug is revealed because the function msdfs.c:is_msdfs_link_internal() provides a buffer of only 7 bytes to the VFS readlink() call in the case where it is simply trying to test if the link is an msdfs link. Given this 7 byte buf, the function vfs_expand_msdfs.c:expand_msdfs_readlink() does not comply with readlink() semantics. Notably, at the end of the function, it tries to put a string of possibly MAX_PATH length into a buffer that is too small, typically 7 bytes. The safe_strcpy call is what generates the error.
The readlink() semantics are:
1. places the contents of the symbolic link in buf which has bufsiz.
2. does not append a null to buf
3. will truncate the contents to bufsiz characters if buf is too small.
4. returns the number of bytes placed in buf.
In expand_msdfs_readlink(), safe_strcpy() is used to fill the buffer which isn't ideal, especially where the buffer is too small since strcpy requires the dest to be big enough and assumes it should null terminate. This is where the error message occurs. This shouldn't be done with strcpy and buf should be filled with as many bytes as possible and no nul.
Second, readlink() returns the number of bytes placed in the buffer. However, this function tries to do that with strlen() which would fail with a non-null terminated buf or one that was too small to hold the whole target.
I'm a rusty C programmer. Here is a fix that probably could be more elegant but makes expand_msdfs_readlink() comply with readlink() semantics and fixes the problem. Replace this:
safe_strcpy(buf, target, bufsiz-1);
result = (strlen(target) < bufsiz) ? strlen(target) : bufsiz;
memcpy(buf, target, result);
This will return the smaller of bufsiz or the length of the symlink target. It will fill buf with the full symlink target or truncate it to bufsiz. No nul termination, I might need that byte if my link target is MAX_PATH long.
*** Bug 7086 has been marked as a duplicate of this bug. ***
Created attachment 5258 [details]
Patch for master/3.5.0.
Can you check this works for you please ? Should make the readlink call match the POSIX spec.
*** Bug 5998 has been marked as a duplicate of this bug. ***
Created attachment 5263 [details]
git-am fix for 3.5.0.
Created attachment 5264 [details]
git-am fix for 3.4.6.
> Can you check this works for you please ? Should make the readlink call match
> the POSIX spec.
Yes, it looks good and works great. Thanks Jeremy.
Created attachment 5267 [details]
Second part of patch for 3.5.0.
From Volker - please add to 3.5.0 in addition to attachment #5263 [details].
Created attachment 5268 [details]
Second part of patch for 3.4.6.
Re-assigning to Karolin for inclusion in 3.4.6 and 3.5.0.
Pushed to v3-5-test and v3-4-test.
Closing out bug report.