GNOME Bugzilla – Bug 710906
sftp doesn't implement any GFileCreateFlags
Last modified: 2013-11-15 15:21:04 UTC
sftp doesn't implement G_FILE_CREATE_PRIVATE. Secondly, the replace code has a line which can set permissions to 0644 which is odd. Also, the permissions are set to 0777 when replacing a symbolic link with G_FILE_CREATE_REPLACE_DESTINATION which is also odd.
sftp doesn't implement G_FILE_CREATE_REPLACE_DESTINATION either. In fact, it seems to behave as though that flag is set.
Created attachment 258187 [details] [review] sftp: Implement GFileCreateFlags and improve permission handling Set the file permissions to 0600 when G_FILE_CREATE_PRIVATE is specified and the file does not exist or G_FILE_CREATE_REPLACE_DESTINATION is specified. Otherwise, set the file permissions to the permissions of the existing file only if it is a regular file (i.e. don't set the permissions to 777 because the existing destination is actually is a symlink). Otherwise, use the default file permissions by not setting any file permissions. If G_FILE_CREATE_REPLACE_DESTINATION is not specified and the destination file is a symbolic link, replace the destination of the symbolic link by truncating it rather than the symbolic link itself.
Review of attachment 258187 [details] [review]: ::: daemon/gvfsbackendsftp.c @@ +3452,3 @@ + if (g_file_info_has_attribute (info, G_FILE_ATTRIBUTE_UNIX_MODE) && + g_file_info_get_file_type (info) == G_FILE_TYPE_REGULAR && + !(op_job->flags & G_FILE_CREATE_REPLACE_DESTINATION)) Same check needs to happen with set_ownership. I.e. we only copy uid/gid if !REPLACE_DESTINATION. @@ +3481,3 @@ + /* If G_FILE_CREATE_REPLACE_DESTINATION is not specified and the destination + * is a symlink, replace the target of the symlink by truncating. */ This means we're removing the original in the case of a hard link. Its unfortunate that n_links is not in the sftp protocol. But it seems safer to use a rename-temp-over-dest in most cases for a slight risk of breaking hard links.
Created attachment 259855 [details] [review] sftp: Implement GFileCreateFlags and improve permission handling Set the file permissions to 0600 when G_FILE_CREATE_PRIVATE is specified and the file does not exist or G_FILE_CREATE_REPLACE_DESTINATION is specified. Otherwise, set the file permissions and ownership to the permissions and ownership of the existing file only if it is a regular file (i.e. don't set the permissions to 777 because the existing destination is actually is a symlink). Otherwise, use the default file permissions by not setting any file permissions. If G_FILE_CREATE_REPLACE_DESTINATION is not specified and the destination file is a symbolic link, replace the destination of the symbolic link by truncating it rather than the symbolic link itself.
The updated patch adds a check for REPLACE_DESTINATION when setting ownership, as per the review.
Review of attachment 259855 [details] [review]: ::: daemon/gvfsbackendsftp.c @@ +3460,3 @@ + if (g_file_info_has_attribute (info, G_FILE_ATTRIBUTE_UNIX_UID) && + g_file_info_has_attribute (info, G_FILE_ATTRIBUTE_UNIX_GID) && + g_file_info_get_file_type (info) == G_FILE_TYPE_REGULAR && Is there any particular reason for the type == REGULAR checks here? Like, i realize we handle symlinks differently below so need not set perms/ownership in that case. But that could be a is_symlink check like below. But what about other cases? I.e. !replace_destination a device node? A fifo? Actually maybe its the is_symlink case below that is wrong. If you're writing to a fifo with !replace_destination we should maybe be using the truncate codepath...
Created attachment 259860 [details] [review] sftp: Implement GFileCreateFlags and improve permission handling Set the file permissions to 0600 when G_FILE_CREATE_PRIVATE is specified and the file does not exist or G_FILE_CREATE_REPLACE_DESTINATION is specified. Otherwise, set the file permissions and ownership to the permissions and ownership of the existing file only if it is a regular file (i.e. don't set the permissions to 777 because the existing destination is actually is a symlink). Otherwise, use the default file permissions by not setting any file permissions. If G_FILE_CREATE_REPLACE_DESTINATION is specified or the destination is a regular file, write to a tempfile and rename; otherwise, replace by truncating the destination.
OK, so the updated patch: * uses the tempfile method if replace_destination is specified or it is a regular file. If replace_destination wasn't specified, the ownership and permissions are maintained. * uses the truncate method otherwise. I hope this takes care of all situations.
Review of attachment 259860 [details] [review]: looks good
Pushed to master as 26163e67d5b31812c58ca661f57a8ab17b0d197b. Thanks for the reviews.