Created attachment 6802 [details] patch Previous rename op of the Netatalk VFS module just removed the original AppleDouble file if any. This is obviously non optimal, this patch addresses this. It will move the AppleDouble file to the corresponding location of the rename target which is "targetdir/.AppleDouble/targetfile".
Ok, I'm evaluating this patch and came across : + become_root(); + ret = rename(src, dst); + unbecome_root(); I'm not happy with that :-). If the rename on the original file succeeded via the VFS_RENAME op, why are you becoming root before doing the rename. That looks incredibly dangerous (and a security hole/race condition) to me. Jeremy.
(In reply to comment #1) > Ok, I'm evaluating this patch ... Thanks! > ...and came across : > > + become_root(); > + ret = rename(src, dst); > + unbecome_root(); > > I'm not happy with that :-). I'm neither, but this now just moves as root instead of deleting as root. ;) > If the rename on the original file succeeded via > the VFS_RENAME op, why are you becoming root before doing the rename. Because permissions on .AppleDouble directories sometimes are not what they should be, so eg the rename of the base file succeeded but the rename of the AppleDouble file might fail with EPERM. Moving as root somewhat hides this lack of specification and implementation robustness wrt controlling permission on the .AppleDouble directories. > That > looks incredibly dangerous (and a security hole/race condition) to me. I can change it of course. But then, what do we do if the rename fails ? Delete as root ? ;) Thanks!
Created attachment 6928 [details] Enhance VFS rename op to rename AppleDouble file Changes from previous patch: * don't become root before renaming adouble file
Ok, I've done a thorough review of this patch and I do have a few questions before I commit something. The atalk_path() function take a bool isdir parameter, but then never calls it other than with a "false" value in the S_ISREG file case. Can that be removed until it actually is called to apply to directories ? Also, atalk_path() should probably just return a char * instead of an int, as we only ever return a 0 or -1 value here. Yeah I know it makes it look like atalk_build_paths() but I don't like the function signature on that either :-). Secondly this module does need a bit of a re-write in that it calls directly into lstat, mkdir and other underlying functions. We do need to fix this so these go through the VFS in order to make this correctly stackable, but that's a fix for another bug I think :-). Can you give me feedback asap on these points. Sorry it's taken so long but I do want to get this code nailed down and committed. Probably will be in 3.6.2, as it's too late for 3.6.1 but I really appreciate the work you've done on this. Jeremy.
(In reply to comment #4) > Ok, I've done a thorough review of this patch and I do have a few questions > before I commit something. Thanks a lot! > The atalk_path() function take a bool isdir parameter, but then never calls it > other than with a "false" value in the S_ISREG file case. Can that be removed > until it actually is called to apply to directories ? > > Also, atalk_path() should probably just return a char * instead of an int, as > we only ever return a 0 or -1 value here. Yeah I know it makes it look like > atalk_build_paths() but I don't like the function signature on that either :-). Yes, bad API. I've done a clean (hopefully, comments welcome) redisign of atalk_build_paths() which would also make the new atalk_path() unnecessary: ---v--- /** * Construct path to AppleDouble file or directory * * The path to the AppleDouble object is constructed according to the * parameter "adflags": * * ADOUBLE_PATH_STAT: * stat() the object in order to determine whether it is a file or directory, * then build path accordingly: * file: path/.AppleDouble/fname * directory: path/fname/.AppleDouble * * ADOUBLE_PATH_STAT | ADOUBLE_PATH_PARENT: * stat() the object in order to determine whether it is a file or directory, * then build path accordingly: * file: path/.AppleDouble/fname * directory: path/fname/.AppleDouble/.Parent * * ADOUBLE_PATH_FILE: * Build path for a file: * path/.AppleDouble/fname * * ADOUBLE_PATH_DIR: * Build AppleDouble directory path for a directory: * path/fname/.AppleDouble * * ADOUBLE_PATH_DIR | ADOUBLE_PATH_PARENT: * Build path to AppleDouble file of the directory itself: * path/fname/.AppleDouble/.Parent * * @param ctx (r) TALLOC_CTX * @param path (r) basepath of requested object * @param fname (r) objects name * @param adflags (r) Conversion flags, see above * @param orig_path (w) Result: combined full path to object (path/fname) * @param adlb_path (w) Result: Path to requested AppleDouble object * @param orig_info (w) Result: sys_lstat(orig_path) * * @return 0 on sucess, -1 on error **/ static int atalk_build_paths(TALLOC_CTX *ctx, const char *path, const char *fname, char **orig_path, char **adbl_path, SMB_STRUCT_STAT *orig_info) ---^--- I'll update the bugreport with a new patch once that's done, probably tomorrow. > Secondly this module does need a bit of a re-write in that it calls directly > into lstat, mkdir and other underlying functions. We do need to fix this so > these go through the VFS in order to make this correctly stackable, but that's > a fix for another bug I think :-). Wouldn't that cause recursion and other unwanted effects because VFS modules are called where they possibly shouldn't ?
(In reply to comment #5) > static int atalk_build_paths(TALLOC_CTX *ctx, > const char *path, > const char *fname, > char **orig_path, > char **adbl_path, > SMB_STRUCT_STAT *orig_info) This should have been static int atalk_build_paths(TALLOC_CTX *ctx, const char *path, const char *fname, unsigned int adflags, char **orig_path, char **adbl_path, SMB_STRUCT_STAT *orig_info)
Actually I'd rather move this module to using smb_filename structs than raw paths - it will make calling standard VFS calls easier. Actually we can quite easily make this module stackable and cause it to call directly into the VFS - by adding a simple check to the top of each VFS function: Firstly we call ret = SMB_VFS_NEXT_XXXX() where XXXX is the VFS function we're implementing. Then for functions that take a path we add: if (strstr_m(path, APPLEDOUBLE) || strstr_m(fname, APPLEDOUBLE)) { return ret; } before doing any more. That way we can quite correctly operate on paths containing .AppleDouble and still go through the full VFS stack.
(In reply to comment #7) > Actually I'd rather move this module to using smb_filename structs than raw > paths - it will make calling standard VFS calls easier. Ok. I've finished an implementation of atalk_build_path() with this signature: /** * Construct path to AppleDouble file or directory * * The path to the AppleDouble object is constructed according to the * parameter "adflags". Example given "example/path": * * ADOUBLE_PATH_STAT: * stat() the object in order to determine whether it is a file or directory, * then build path accordingly: * file: example/.AppleDouble/path * directory: example/path/.AppleDouble/ * * ADOUBLE_PATH_STAT | ADOUBLE_PATH_PARENT: * stat() the object in order to determine whether it is a file or directory, * then build path accordingly: * file: example/.AppleDouble/path * directory: example/path/.AppleDouble/.Parent * * ADOUBLE_PATH_FILE: * Build path for a file: * example/.AppleDouble/path * * ADOUBLE_PATH_DIR: * Build AppleDouble directory path for a directory: * example/path/.AppleDouble/ * * ADOUBLE_PATH_DIR | ADOUBLE_PATH_PARENT: * Build path to AppleDouble file of the directory itself: * example/path/.AppleDouble/.Parent * * @param handle (r) VFS handle * @param ctx (r) TALLOC_CTX * @param path (r) pathname to filesystem object * @param adflags (r) Conversion flags, see above * * @return AppleDouble path on sucess, NULL on error **/ static struct smb_filename *atalk_build_paths(const struct vfs_handle_struct *handle, TALLOC_CTX *ctx, struct smb_filename *path, unsigned int adflags); I'm now adjusting the rest of the code to make use of it. I'm using dirname() and basename() in there on talloc_strdup'ed strings. I had to include libgen.h as it seems to ne be included in the default includes. Also I noticed that the whole code is neither using dirname() nor basename(). Is there a reason ? I can roll my own, but... > Actually we can quite easily make this module stackable and cause it to call > directly into the VFS - by adding a simple check to the top of each VFS > function: > > Firstly we call > > ret = SMB_VFS_NEXT_XXXX() > > where XXXX is the VFS function we're implementing. Then for functions that take > a path we add: > > if (strstr_m(path, APPLEDOUBLE) || strstr_m(fname, APPLEDOUBLE)) { > return ret; > } > > before doing any more. That way we can quite correctly operate on paths > containing .AppleDouble and still go through the full VFS stack. I'll also add that. Once I'm done, I'll attach a complete patch. Thanks!
Created attachment 7016 [details] atalk_build_paths() rewrite and VFS rename op implementation As promised: Rewrite atalk_build_paths() to make use of struct smb_filename instead of plain char *. Rewrite all callers of atalk_build_paths() to accomodate this. Also fixed: Implement rename() VFS function. Instead of using individual TALLOC contexts, use talloc_tos() context. atalk_lchown() called SMB_VFS_NEXT_CHOWN instead of SMB_VFS_NEXT_LCHOWN. Avoid excessive gotos.
(In reply to comment #9) > Created attachment 7016 [details] > atalk_build_paths() rewrite and VFS rename op implementation > > As promised: > > Rewrite atalk_build_paths() to make use of struct smb_filename instead of plain > char *. > Rewrite all callers of atalk_build_paths() to accomodate this. > Also fixed: > Implement rename() VFS function. > Instead of using individual TALLOC contexts, use talloc_tos() context. > atalk_lchown() called SMB_VFS_NEXT_CHOWN instead of SMB_VFS_NEXT_LCHOWN. > Avoid excessive gotos. adding myself to the CC list. This will impact Small Tree since we're looking to use FreeBSD and ZFS, so we'll have Apple double files.
Ralph Böhme wrote a new "vfs fruit" module which will hopefully make it into 4.2, which also handles cases like this gracefully. I think vfs netatalk is quite deprecated and won't see any more changes. Jeremy: are you have been asked for a review here I leave it up to you to review+ this or to close the bug as wontfix :-)