Merge pull request #950 from Hinidu/os_fchown

Implement os_fchown and remove HAVE_FCHOWN

.ci/clang-asan.sh

@@ -12,10 +12,8 @@ if [ ! -d /usr/local/clang-$clang_version ]; then
 	sudo mkdir /usr/local/clang-$clang_version
 	wget -q -O - http://llvm.org/releases/$clang_version/clang+llvm-$clang_version-x86_64-unknown-ubuntu12.04.xz \
 		| sudo tar xJf - --strip-components=1 -C /usr/local/clang-$clang_version
-	export CC=/usr/local/clang-$clang_version/bin/clang
-else
-	export CC=clang
 fi
+export CC=/usr/local/clang-$clang_version/bin/clang
 symbolizer=/usr/local/clang-$clang_version/bin/llvm-symbolizer
 
 export SANITIZE=1

.travis.yml

@@ -13,5 +13,13 @@ env:
     - CI_TARGET=clint
     - CI_TARGET=api-python
     - CI_TARGET=coverity
+before_install:
+  # Adds user to a dummy group.
+  # That allows to test changing the group of the file by `os_fchown`.
+  - sudo groupadd chown_test
+  - sudo usermod -a -G chown_test ${USER}
 script:
-  - sh -e "${CI_SCRIPTS}/${CI_TARGET}.sh"
+  # This will pass the environment variables down to a bash process which runs
+  # as $USER, while retaining the environment variables defined and belonging
+  # to secondary groups given above in usermod.
+  - sudo -E su ${USER} -c "sh -e \"${CI_SCRIPTS}/${CI_TARGET}.sh\""

config/CMakeLists.txt

@@ -38,7 +38,6 @@ check_include_files(unistd.h HAVE_UNISTD_H)
 check_include_files(utime.h HAVE_UTIME_H)
 
 # Functions
-check_function_exists(fchown HAVE_FCHOWN)
 check_function_exists(fseeko HAVE_FSEEKO)
 check_function_exists(fsync HAVE_FSYNC)
 check_function_exists(getpwent HAVE_GETPWENT)

config/config.h.in

@@ -18,7 +18,6 @@
 #cmakedefine HAVE__NSGETENVIRON
 #cmakedefine HAVE_CRT_EXTERNS_H
 #cmakedefine HAVE_DIRENT_H
-#cmakedefine HAVE_FCHOWN
 #cmakedefine HAVE_FCNTL_H
 #cmakedefine HAVE_FD_CLOEXEC
 #cmakedefine HAVE_FSEEKO

src/nvim/ex_cmds.c

@@ -1606,13 +1606,13 @@ void write_viminfo(char_u *file, int forceit)
           fp_out = mch_fopen((char *)tempname, WRITEBIN);
       }
 
-#if defined(UNIX) && defined(HAVE_FCHOWN)
+#ifdef UNIX
       /*
        * Make sure the owner can read/write it.  This only works for
        * root.
        */
       if (fp_out != NULL) {
-        fchown(fileno(fp_out), old_info.stat.st_uid, old_info.stat.st_gid);
+        os_fchown(fileno(fp_out), old_info.stat.st_uid, old_info.stat.st_gid);
       }
 #endif
     }

src/nvim/fileio.c

@@ -2711,16 +2711,10 @@ buf_write (
        * - it's a hard link
        * - it's a symbolic link
        * - we don't have write permission in the directory
-       * - we can't set the owner/group of the new file
        */
       if (file_info_old.stat.st_nlink > 1
           || !os_get_file_info_link((char *)fname, &file_info)
-          || !os_file_info_id_equal(&file_info, &file_info_old)
+          || !os_file_info_id_equal(&file_info, &file_info_old)) {
-#  ifndef HAVE_FCHOWN
-          || file_info.stat.st_uid != file_info_old.stat.st_uid
-          || file_info.stat.st_gid != file_info_old.stat.st_gid
-#  endif
-          ) {
         backup_copy = TRUE;
       } else
 # endif
@@ -2744,9 +2738,7 @@ buf_write (
           backup_copy = TRUE;
         else {
 # ifdef UNIX
-#  ifdef HAVE_FCHOWN
+          os_fchown(fd, file_info_old.stat.st_uid, file_info_old.stat.st_gid);
-          fchown(fd, file_info_old.stat.st_uid, file_info_old.stat.st_gid);
-#  endif
           if (!os_get_file_info((char *)IObuff, &file_info)
               || file_info.stat.st_uid != file_info_old.stat.st_uid
               || file_info.stat.st_gid != file_info_old.stat.st_gid
@@ -2909,10 +2901,7 @@ buf_write (
              * others.
              */
             if (file_info_new.stat.st_gid != file_info_old.stat.st_gid
-# ifdef HAVE_FCHOWN  /* sequent-ptx lacks fchown() */
+                && os_fchown(bfd, -1, file_info_old.stat.st_gid) != 0) {
-                && fchown(bfd, (uid_t)-1, file_info_old.stat.st_gid) != 0
-# endif
-                ) {
               os_setperm(backup, (perm & 0707) | ((perm & 07) << 3));
             }
 # ifdef HAVE_SELINUX
@@ -3424,19 +3413,16 @@ restore_backup:
   /* When creating a new file, set its owner/group to that of the original
    * file.  Get the new device and inode number. */
   if (backup != NULL && !backup_copy) {
-# ifdef HAVE_FCHOWN
-
     /* don't change the owner when it's already OK, some systems remove
      * permission or ACL stuff */
     FileInfo file_info;
     if (!os_get_file_info((char *)wfname, &file_info)
         || file_info.stat.st_uid != file_info_old.stat.st_uid
         || file_info.stat.st_gid != file_info_old.stat.st_gid) {
-      fchown(fd, file_info_old.stat.st_uid, file_info_old.stat.st_gid);
+      os_fchown(fd, file_info_old.stat.st_uid, file_info_old.stat.st_gid);
       if (perm >= 0)            /* set permission again, may have changed */
         (void)os_setperm(wfname, perm);
     }
-# endif
     buf_set_file_id(buf);
   } else if (!buf->file_id_valid) {
     // Set the file_id when creating a new file.

src/nvim/os/fs.c

@@ -210,6 +210,21 @@ int os_setperm(const char_u *name, int perm)
   return FAIL;
 }
 
+/// Changes the ownership of the file referred to by the open file descriptor.
+///
+/// @return `0` on success, a libuv error code on failure.
+///
+/// @note If the `owner` or `group` is specified as `-1`, then that ID is not
+/// changed.
+int os_fchown(int file_descriptor, uv_uid_t owner, uv_gid_t group)
+{
+  uv_fs_t request;
+  int result = uv_fs_fchown(uv_default_loop(), &request, file_descriptor,
+                            owner, group, NULL);
+  uv_fs_req_cleanup(&request);
+  return result;
+}
+
 /// Check if a file exists.
 ///
 /// @return `true` if `name` exists.

src/nvim/undo.c

@@ -1119,10 +1119,7 @@ void u_write_undo(char_u *name, int forceit, buf_T *buf, char_u *hash)
   if (os_get_file_info((char *)buf->b_ffname, &file_info_old)
       && os_get_file_info((char *)file_name, &file_info_new)
       && file_info_old.stat.st_gid != file_info_new.stat.st_gid
-# ifdef HAVE_FCHOWN  /* sequent-ptx lacks fchown() */
+      && os_fchown(fd, -1, file_info_old.stat.st_gid) != 0) {
-      && fchown(fd, (uid_t)-1, file_info_old.stat.st_gid) != 0
-# endif
-      ) {
     os_setperm(file_name, (perm & 0707) | ((perm & 07) << 3));
   }
 # ifdef HAVE_SELINUX

test/unit/os/fs_spec.moon

@@ -112,6 +112,12 @@ describe 'fs function', ->
     os_setperm = (filename, perm) ->
       fs.os_setperm (to_cstr filename), perm
 
+    os_fchown = (filename, user_id, group_id) ->
+      fd = ffi.C.open filename, 0
+      res = fs.os_fchown fd, user_id, group_id
+      ffi.C.close fd
+      return res
+
     os_file_is_readonly = (filename) ->
       fs.os_file_is_readonly (to_cstr filename)
 
@@ -158,6 +164,43 @@ describe 'fs function', ->
         perm = ffi.C.kS_IXUSR
         eq FAIL, (os_setperm 'non-existing-file', perm)
 
+    describe 'os_fchown', ->
+      filename = 'unit-test-directory/test.file'
+
+      it 'does not change owner and group if respective IDs are equal to -1', ->
+        uid = lfs.attributes filename, 'uid'
+        gid = lfs.attributes filename, 'gid'
+        eq 0, os_fchown filename, -1, -1
+        eq uid, lfs.attributes filename, 'uid'
+        eq gid, lfs.attributes filename, 'gid'
+
+      it 'owner of a file may change the group of the file
+      to any group of which that owner is a member', ->
+        -- Some systems may not have `id` utility.
+        if (os.execute('id -G &> /dev/null') == 0)
+          file_gid = lfs.attributes filename, 'gid'
+
+          -- Gets ID of any group of which current user is a member except the
+          -- group that owns the file.
+          id_fd = io.popen('id -G')
+          new_gid = id_fd\read '*n'
+          if (new_gid == file_gid)
+            new_gid = id_fd\read '*n'
+          id_fd\close!
+
+          -- User can be a member of only one group.
+          -- In that case we can not perform this test.
+          if new_gid
+            eq 0, (os_fchown filename, -1, new_gid)
+            eq new_gid, (lfs.attributes filename, 'gid')
+
+      it 'returns nonzero if process has not enough permissions', ->
+        -- On Windows `os_fchown` always returns 0
+        -- because `uv_fs_chown` is no-op on this platform.
+        if (ffi.os != 'Windows' and ffi.C.geteuid! != 0)
+          -- chown to root
+          neq 0, os_fchown filename, 0, 0
+
     describe 'os_file_is_readonly', ->
       it 'returns true if the file is readonly', ->
         perm = os_getperm 'unit-test-directory/test.file'