Fix the chown syscall with negative id

src/libos/src/fs/file_ops/chown.rs

@@ -7,12 +7,19 @@ bitflags! {
     }
 }
 
-pub fn do_fchownat(fs_path: &FsPath, uid: u32, gid: u32, flags: ChownFlags) -> Result<()> {
+pub fn do_fchownat(fs_path: &FsPath, uid: i32, gid: i32, flags: ChownFlags) -> Result<()> {
     debug!(
         "fchownat: fs_path: {:?}, uid: {}, gid: {}, flags: {:?}",
         fs_path, uid, gid, flags
     );
 
+    let uid = to_opt(uid)?;
+    let gid = to_opt(gid)?;
+    // Return early if owner and group are -1
+    if uid.is_none() && gid.is_none() {
+        return Ok(());
+    }
+
     let inode = {
         let path = fs_path.to_abs_path()?;
         let current = current!();
@@ -24,19 +31,47 @@ pub fn do_fchownat(fs_path: &FsPath, uid: u32, gid: u32, flags: ChownFlags) -> R
         }
     };
     let mut info = inode.metadata()?;
+    if let Some(uid) = uid {
         info.uid = uid as usize;
+    }
+    if let Some(gid) = gid {
         info.gid = gid as usize;
+    }
     inode.set_metadata(&info)?;
     Ok(())
 }
 
-pub fn do_fchown(fd: FileDesc, uid: u32, gid: u32) -> Result<()> {
+pub fn do_fchown(fd: FileDesc, uid: i32, gid: i32) -> Result<()> {
     debug!("fchown: fd: {}, uid: {}, gid: {}", fd, uid, gid);
 
+    let uid = to_opt(uid)?;
+    let gid = to_opt(gid)?;
+    // Return early if owner and group are -1
+    if uid.is_none() && gid.is_none() {
+        return Ok(());
+    }
+
     let file_ref = current!().file(fd)?;
     let mut info = file_ref.metadata()?;
+    if let Some(uid) = uid {
         info.uid = uid as usize;
+    }
+    if let Some(gid) = gid {
         info.gid = gid as usize;
+    }
     file_ref.set_metadata(&info)?;
     Ok(())
 }
+
+fn to_opt(id: i32) -> Result<Option<u32>> {
+    let id = if id >= 0 {
+        Some(id as u32)
+    } else if id == -1 {
+        // If the ID is specified as -1, then that ID is not changed
+        None
+    } else {
+        return_errno!(EINVAL, "invalid id");
+    };
+
+    Ok(id)
+}

src/libos/src/fs/syscalls.rs

@@ -606,16 +606,16 @@ pub fn do_fchmodat(dirfd: i32, path: *const i8, mode: u16) -> Result<isize> {
     Ok(0)
 }
 
-pub fn do_chown(path: *const i8, uid: u32, gid: u32) -> Result<isize> {
+pub fn do_chown(path: *const i8, uid: i32, gid: i32) -> Result<isize> {
     self::do_fchownat(AT_FDCWD, path, uid, gid, 0)
 }
 
-pub fn do_fchown(fd: FileDesc, uid: u32, gid: u32) -> Result<isize> {
+pub fn do_fchown(fd: FileDesc, uid: i32, gid: i32) -> Result<isize> {
     file_ops::do_fchown(fd, uid, gid)?;
     Ok(0)
 }
 
-pub fn do_fchownat(dirfd: i32, path: *const i8, uid: u32, gid: u32, flags: i32) -> Result<isize> {
+pub fn do_fchownat(dirfd: i32, path: *const i8, uid: i32, gid: i32, flags: i32) -> Result<isize> {
     let path = from_user::clone_cstring_safely(path)?
         .to_string_lossy()
         .into_owned();
@@ -631,7 +631,7 @@ pub fn do_fchownat(dirfd: i32, path: *const i8, uid: u32, gid: u32, flags: i32)
     Ok(0)
 }
 
-pub fn do_lchown(path: *const i8, uid: u32, gid: u32) -> Result<isize> {
+pub fn do_lchown(path: *const i8, uid: i32, gid: i32) -> Result<isize> {
     self::do_fchownat(
         AT_FDCWD,
         path,

src/libos/src/syscall/mod.rs

@@ -189,9 +189,9 @@ macro_rules! process_syscall_table_with_callback {
             (Readlink = 89) => do_readlink(path: *const i8, buf: *mut u8, size: usize),
             (Chmod = 90) => do_chmod(path: *const i8, mode: u16),
             (Fchmod = 91) => do_fchmod(fd: FileDesc, mode: u16),
-            (Chown = 92) => do_chown(path: *const i8, uid: u32, gid: u32),
-            (Fchown = 93) => do_fchown(fd: FileDesc, uid: u32, gid: u32),
-            (Lchown = 94) => do_lchown(path: *const i8, uid: u32, gid: u32),
+            (Chown = 92) => do_chown(path: *const i8, uid: i32, gid: i32),
+            (Fchown = 93) => do_fchown(fd: FileDesc, uid: i32, gid: i32),
+            (Lchown = 94) => do_lchown(path: *const i8, uid: i32, gid: i32),
             (Umask = 95) => do_umask(mask: u16),
             (Gettimeofday = 96) => do_gettimeofday(tv_u: *mut timeval_t),
             (Getrlimit = 97) => do_gettrlimit(resource: u32, rlim: *mut rlimit_t),
@@ -357,7 +357,7 @@ macro_rules! process_syscall_table_with_callback {
             (Openat = 257) => do_openat(dirfd: i32, path: *const i8, flags: u32, mode: u16),
             (Mkdirat = 258) => do_mkdirat(dirfd: i32, path: *const i8, mode: u16),
             (Mknodat = 259) => handle_unsupported(),
-            (Fchownat = 260) => do_fchownat(dirfd: i32, path: *const i8, uid: u32, gid: u32, flags: i32),
+            (Fchownat = 260) => do_fchownat(dirfd: i32, path: *const i8, uid: i32, gid: i32, flags: i32),
             (Futimesat = 261) => do_futimesat(dirfd: i32, path: *const i8, times: *const timeval_t),
             (Fstatat = 262) => do_fstatat(dirfd: i32, path: *const i8, stat_buf: *mut Stat, flags: u32),
             (Unlinkat = 263) => do_unlinkat(dirfd: i32, path: *const i8, flags: i32),

test/chown/main.c

@@ -55,6 +55,57 @@ static int __test_chown(const char *file_path) {
     return 0;
 }
 
+static int __test_chown_with_negative_id(const char *file_path) {
+    struct stat old_stat_buf;
+    struct stat new_stat_buf;
+    uid_t uid = -100;
+    gid_t gid = -100;
+    int ret;
+
+    ret = chown(file_path, uid, gid);
+    if (!(ret < 0 && errno == EINVAL)) {
+        THROW_ERROR("chown should return EINVAL");
+    }
+
+    ret = stat(file_path, &old_stat_buf);
+    if (ret < 0) {
+        THROW_ERROR("failed to stat file");
+    }
+
+    uid = 100;
+    gid = -1;
+    ret = chown(file_path, uid, gid);
+    if (ret < 0) {
+        THROW_ERROR("failed to chown file");
+    }
+
+    ret = stat(file_path, &new_stat_buf);
+    if (ret < 0) {
+        THROW_ERROR("failed to stat file");
+    }
+    if (new_stat_buf.st_uid != uid || new_stat_buf.st_gid != old_stat_buf.st_gid) {
+        THROW_ERROR("check chown result failed");
+    }
+
+    old_stat_buf.st_uid = new_stat_buf.st_uid;
+    uid = -1;
+    gid = 100;
+    ret = chown(file_path, uid, gid);
+    if (ret < 0) {
+        THROW_ERROR("failed to chown file");
+    }
+
+    ret = stat(file_path, &new_stat_buf);
+    if (ret < 0) {
+        THROW_ERROR("failed to stat file");
+    }
+    if (new_stat_buf.st_uid != old_stat_buf.st_uid || new_stat_buf.st_gid != gid) {
+        THROW_ERROR("check chown result failed");
+    }
+
+    return 0;
+}
+
 static int __test_lchown(const char *file_path) {
     struct stat stat_buf;
     uid_t uid = 100;
@@ -188,6 +239,10 @@ static int test_chown() {
     return test_chown_framework(__test_chown);
 }
 
+static int test_chown_with_negative_id() {
+    return test_chown_framework(__test_chown_with_negative_id);
+}
+
 static int test_lchown() {
     return test_chown_framework(__test_lchown);
 }
@@ -210,6 +265,7 @@ static int test_fchownat_with_empty_path() {
 
 static test_case_t test_cases[] = {
     TEST_CASE(test_chown),
+    TEST_CASE(test_chown_with_negative_id),
     TEST_CASE(test_lchown),
     TEST_CASE(test_fchown),
     TEST_CASE(test_fchownat),