[LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

[LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

[Xie Ziyao]

For this:
  testcases/kernel/syscalls/chown/chown03.c

Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
v2->v3:
1. Remove some unnecessary comments and incorrect output prints;
2. Moved some prerequisite codes from the setup() function to the run() function
and add code logic for checking whether the setting is successful.

 testcases/kernel/syscalls/chown/chown03.c | 241 +++++++---------------
 1 file changed, 70 insertions(+), 171 deletions(-)

diff --git a/testcases/kernel/syscalls/chown/chown03.c b/testcases/kernel/syscalls/chown/chown03.c
index 2c7bcfe7d..4469f1c4d 100644
--- a/testcases/kernel/syscalls/chown/chown03.c
+++ b/testcases/kernel/syscalls/chown/chown03.c
@@ -1,72 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *
- *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ * Copyright (c) International Business Machines  Corp., 2001
+ * 07/2001 Ported by Wayne Boyer
  */

-/*
- * Test Name: chown03
- *
- * Test Description:
- *  Verify that, chown(2) succeeds to change the group of a file specified
- *  by path when called by non-root user with the following constraints,
- *	- euid of the process is equal to the owner of the file.
- *	- the intended gid is either egid, or one of the supplementary gids
- *	  of the process.
- *  Also, verify that chown() clears the setuid/setgid bits set on the file.
- *
- * Expected Result:
- *  chown(2) should return 0 and the ownership set on the file should match
- *  the numeric values contained in owner and group respectively.
- *
- * Algorithm:
- *  Setup:
- *   Setup signal handling.
- *   Create temporary directory.
- *   Pause for SIGUSR1 if option specified.
- *
- *  Test:
- *   Loop if the proper options are given.
- *   Execute system call
- *   Check return code, if system call failed (return=-1)
- *	Log the errno and Issue a FAIL message.
- *   Otherwise,
- *	Verify the Functionality of system call
- *      if successful,
- *		Issue Functionality-Pass message.
- *      Otherwise,
- *		Issue Functionality-Fail message.
- *  Cleanup:
- *   Print errno log and/or timing stats if options given
- *   Delete the temporary directory created.
- *
- * Usage:  <for command-line>
- *  chown03 [-c n] [-f] [-i n] [-I x] [-P x] [-t]
- *     where,  -c n : Run n copies concurrently.
- *             -f   : Turn off functionality Testing.
- *	       -i n : Execute test n times.
- *	       -I x : Execute test for x seconds.
- *	       -P x : Pause for x seconds between iterations.
- *	       -t   : Turn on syscall timing.
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *
- * RESTRICTIONS:
+/*\
+ * [Description]
  *
+ * Verify that, chown(2) succeeds to change the group of a file specified
+ * by path when called by non-root user with the following constraints,
+ * - euid of the process is equal to the owner of the file.
+ * - the intended gid is either egid, or one of the supplementary gids
+ *   of the process.
+ * Also, verify that chown() clears the setuid/setgid bits set on the file.
  */

 #include <stdio.h>
@@ -80,123 +26,76 @@
 #include <grp.h>
 #include <pwd.h>

-#include "test.h"
-#include "safe_macros.h"
-#include "compat_16.h"
+#include "tst_test.h"
+#include "compat_tst_16.h"

-#define FILE_MODE	(S_IFREG|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define NEW_PERMS	(S_IFREG|S_IRWXU|S_IRWXG|S_ISUID|S_ISGID)
-#define TESTFILE	"testfile"
+#define FILE_MODE (S_IFREG|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
+#define NEW_PERMS (S_IFREG|S_IRWXU|S_IRWXG|S_ISUID|S_ISGID)
+#define FILENAME "chown03_testfile"

-TCID_DEFINE(chown03);
-int TST_TOTAL = 1;		/* Total number of test conditions */
-char nobody_uid[] = "nobody";
 struct passwd *ltpuser;

-void setup();			/* setup function for the test */
-void cleanup();			/* cleanup function for the test */
-
-int main(int ac, char **av)
+static void run(void)
 {
-	struct stat stat_buf;	/* stat(2) struct contents */
-	int lc;
-	uid_t user_id;		/* Owner id of the test file. */
-	gid_t group_id;		/* Group id of the test file. */
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		UID16_CHECK((user_id = geteuid()), "chown", cleanup)
-		GID16_CHECK((group_id = getegid()), "chown", cleanup)
-
-		TEST(CHOWN(cleanup, TESTFILE, -1, group_id));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL | TTERRNO, "chown(%s, ..) failed",
-				 TESTFILE);
-			continue;
-		}
-
-		if (stat(TESTFILE, &stat_buf) == -1)
-			tst_brkm(TFAIL | TERRNO, cleanup,
-				 "stat failed");
-
-		if (stat_buf.st_uid != user_id ||
-		    stat_buf.st_gid != group_id)
-			tst_resm(TFAIL, "%s: Incorrect ownership"
-				 "set to %d %d, Expected %d %d",
-				 TESTFILE, stat_buf.st_uid,
-				 stat_buf.st_gid, user_id, group_id);
-
-		if (stat_buf.st_mode !=
-		    (NEW_PERMS & ~(S_ISUID | S_ISGID)))
-			tst_resm(TFAIL, "%s: incorrect mode permissions"
-				 " %#o, Expected %#o", TESTFILE,
-				 stat_buf.st_mode,
-				 NEW_PERMS & ~(S_ISUID | S_ISGID));
-		else
-			tst_resm(TPASS, "chown(%s, ..) was successful",
-				 TESTFILE);
-	}
-
-	cleanup();
-	tst_exit();
+    SAFE_SETEUID(0);
+    SAFE_CHOWN(FILENAME, -1, 0);
+    SAFE_CHMOD(FILENAME, NEW_PERMS);
+    SAFE_SETEUID(ltpuser->pw_uid);
+
+    uid_t uid;
+    gid_t gid;
+    UID16_CHECK((uid = geteuid()), "chown");
+    GID16_CHECK((gid = getegid()), "chown");
+
+    struct stat stat_buf;
+    SAFE_STAT(FILENAME, &stat_buf);
+    if (stat_buf.st_uid != uid || stat_buf.st_gid != 0)
+        tst_res(TFAIL, "%s: Incorrect ownership"
+                       "set to %d %d, Expected %d %d",
+                FILENAME, stat_buf.st_uid,
+                stat_buf.st_gid, uid, 0);
+
+    TST_EXP_PASS(CHOWN(FILENAME, -1, gid), "chown(%s,%d,%d)",
+                 FILENAME, -1, gid);
+
+    SAFE_STAT(FILENAME, &stat_buf);
+    if (stat_buf.st_uid != uid || stat_buf.st_gid != gid)
+        tst_res(TFAIL, "%s: Incorrect ownership"
+                       "set to %d %d, Expected %d %d",
+                FILENAME, stat_buf.st_uid,
+                stat_buf.st_gid, uid, gid);
+
+    if (stat_buf.st_mode != (NEW_PERMS & ~(S_ISUID | S_ISGID)))
+        tst_res(TFAIL, "%s: incorrect mode permissions"
+                       " %#o, Expected %#o", FILENAME,
+                stat_buf.st_mode,
+                NEW_PERMS & ~(S_ISUID | S_ISGID));
+    else
+        tst_res(TPASS, "chown(%s, ..)", FILENAME);
 }

-/*
- * void
- * setup() - performs all ONE TIME setup for this test.
- *  Create a temporary directory and change directory to it.
- *  Create a test file under temporary directory and close it
- *  Change the group ownership on testfile.
- */
-void setup(void)
+static void setup(void)
 {
-	int fd;			/* file handler for testfile */
-
-	TEST_PAUSE;
-
-	tst_require_root();
-
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-
-	tst_tmpdir();
-
-	ltpuser = getpwnam(nobody_uid);
-	if (ltpuser == NULL)
-		tst_brkm(TBROK | TERRNO, NULL, "getpwnam(\"nobody\") failed");
-	SAFE_SETEGID(NULL, ltpuser->pw_gid);
-	SAFE_SETEUID(NULL, ltpuser->pw_uid);
-
-	/* Create a test file under temporary directory */
-	if ((fd = open(TESTFILE, O_RDWR | O_CREAT, FILE_MODE)) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "open(%s, O_RDWR|O_CREAT, %o) failed", TESTFILE,
-			 FILE_MODE);
+    int fd;

-	SAFE_SETEUID(cleanup, 0);
+    ltpuser = SAFE_GETPWNAM("nobody");
+    SAFE_SETEGID(ltpuser->pw_gid);
+    SAFE_SETEUID(ltpuser->pw_uid);

-	SAFE_FCHOWN(cleanup, fd, -1, 0);
-
-	SAFE_FCHMOD(cleanup, fd, NEW_PERMS);
-
-	SAFE_SETEUID(cleanup, ltpuser->pw_uid);
-
-	SAFE_CLOSE(cleanup, fd);
+    fd = SAFE_OPEN(FILENAME, O_RDWR | O_CREAT, FILE_MODE);
+    SAFE_CLOSE(fd);
 }

-void cleanup(void)
+static void cleanup(void)
 {
-	if (setegid(0) == -1)
-		tst_resm(TWARN | TERRNO, "setegid(0) failed");
-	if (seteuid(0) == -1)
-		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
-
-	tst_rmdir();
-
+    SAFE_SETEGID(0);
+    SAFE_SETEUID(0);
 }
+
+static struct tst_test test = {
+        .needs_root = 1,
+        .needs_tmpdir = 1,
+        .setup = setup,
+        .cleanup = cleanup,
+        .test_all = run,
+};
--
2.17.1

[LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

[Cyril Hrubis]

Hi!
> +    SAFE_SETEUID(0);
> +    SAFE_CHOWN(FILENAME, -1, 0);
> +    SAFE_CHMOD(FILENAME, NEW_PERMS);
> +    SAFE_SETEUID(ltpuser->pw_uid);

LTP uses Linux kernel conding style which requires tabs for
indentation, while this code uses four spaces.

You can use checkpatch.pl shipped along with Linux kernel sources to
check coding style before sending patches to the list.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

[Alexey Kodanev]

On 19.04.2021 04:34, Xie Ziyao wrote:
> For this:
>   testcases/kernel/syscalls/chown/chown03.c
> 
> Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
> ---
> v2->v3:
> 1. Remove some unnecessary comments and incorrect output prints;
> 2. Moved some prerequisite codes from the setup() function to the run() function
> and add code logic for checking whether the setting is successful.
> 

Hi Ziyao,

>  testcases/kernel/syscalls/chown/chown03.c | 241 +++++++---------------
>  1 file changed, 70 insertions(+), 171 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/chown/chown03.c b/testcases/kernel/syscalls/chown/chown03.c
> index 2c7bcfe7d..4469f1c4d 100644
> --- a/testcases/kernel/syscalls/chown/chown03.c
> +++ b/testcases/kernel/syscalls/chown/chown03.c
> @@ -1,72 +1,18 @@

...
> -int TST_TOTAL = 1;		/* Total number of test conditions */
> -char nobody_uid[] = "nobody";
>  struct passwd *ltpuser;

static struct passwd *ltpuser;

> 
> -void setup();			/* setup function for the test */
> -void cleanup();			/* cleanup function for the test */
> -
> -int main(int ac, char **av)
> +static void run(void)
>  {
...
> +    SAFE_SETEUID(0);
> +    SAFE_CHOWN(FILENAME, -1, 0);
> +    SAFE_CHMOD(FILENAME, NEW_PERMS);
> +    SAFE_SETEUID(ltpuser->pw_uid);
> +
> +    uid_t uid;
> +    gid_t gid;
> +    UID16_CHECK((uid = geteuid()), "chown");
> +    GID16_CHECK((gid = getegid()), "chown");
> +
> +    struct stat stat_buf;
> +    SAFE_STAT(FILENAME, &stat_buf);
> +    if (stat_buf.st_uid != uid || stat_buf.st_gid != 0)
> +        tst_res(TFAIL, "%s: Incorrect ownership"
> +                       "set to %d %d, Expected %d %d",
> +                FILENAME, stat_buf.st_uid,
> +                stat_buf.st_gid, uid, 0);
> +
> +    TST_EXP_PASS(CHOWN(FILENAME, -1, gid), "chown(%s,%d,%d)",
> +                 FILENAME, -1, gid);
> +
> +    SAFE_STAT(FILENAME, &stat_buf);
> +    if (stat_buf.st_uid != uid || stat_buf.st_gid != gid)
> +        tst_res(TFAIL, "%s: Incorrect ownership"
> +                       "set to %d %d, Expected %d %d",
> +                FILENAME, stat_buf.st_uid,
> +                stat_buf.st_gid, uid, gid);

There are two similar owner checks now, it would be better to
move them to a single function, something like this:

static void check_owner(struct stat *s, uid_t exp_uid, gid_t exp_gid)
{
        if (s->st_uid != exp_uid || s->st_gid != exp_gid) {
                tst_res(TFAIL, "%s: wrong owner set to %d %d, expected %d %d",
                        FILENAME, s->st_uid, s->st_gid, exp_uid, exp_gid);
        } else {
                tst_res(TPASS, "%s: expected owner set %d %d",
                        FILENAME, exp_uid, exp_gid);
        }
}

[LTP] [PATCH v4] syscalls/chown: Rewrite chown/chown03.c with the new api

[Xie Ziyao]

For this:
  testcases/kernel/syscalls/chown/chown03.c

Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
v3->v4:
1. Move two similar owner checks to a single function;
2. Modify the incorrect code style.

 testcases/kernel/syscalls/chown/chown03.c | 241 +++++++---------------
 1 file changed, 70 insertions(+), 171 deletions(-)

diff --git a/testcases/kernel/syscalls/chown/chown03.c b/testcases/kernel/syscalls/chown/chown03.c
index 2c7bcfe7d..68e682843 100644
--- a/testcases/kernel/syscalls/chown/chown03.c
+++ b/testcases/kernel/syscalls/chown/chown03.c
@@ -1,72 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *
- *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ * Copyright (c) International Business Machines  Corp., 2001
+ * 07/2001 Ported by Wayne Boyer
  */

-/*
- * Test Name: chown03
- *
- * Test Description:
- *  Verify that, chown(2) succeeds to change the group of a file specified
- *  by path when called by non-root user with the following constraints,
- *	- euid of the process is equal to the owner of the file.
- *	- the intended gid is either egid, or one of the supplementary gids
- *	  of the process.
- *  Also, verify that chown() clears the setuid/setgid bits set on the file.
- *
- * Expected Result:
- *  chown(2) should return 0 and the ownership set on the file should match
- *  the numeric values contained in owner and group respectively.
- *
- * Algorithm:
- *  Setup:
- *   Setup signal handling.
- *   Create temporary directory.
- *   Pause for SIGUSR1 if option specified.
- *
- *  Test:
- *   Loop if the proper options are given.
- *   Execute system call
- *   Check return code, if system call failed (return=-1)
- *	Log the errno and Issue a FAIL message.
- *   Otherwise,
- *	Verify the Functionality of system call
- *      if successful,
- *		Issue Functionality-Pass message.
- *      Otherwise,
- *		Issue Functionality-Fail message.
- *  Cleanup:
- *   Print errno log and/or timing stats if options given
- *   Delete the temporary directory created.
- *
- * Usage:  <for command-line>
- *  chown03 [-c n] [-f] [-i n] [-I x] [-P x] [-t]
- *     where,  -c n : Run n copies concurrently.
- *             -f   : Turn off functionality Testing.
- *	       -i n : Execute test n times.
- *	       -I x : Execute test for x seconds.
- *	       -P x : Pause for x seconds between iterations.
- *	       -t   : Turn on syscall timing.
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *
- * RESTRICTIONS:
+/*\
+ * [Description]
  *
+ * Verify that, chown(2) succeeds to change the group of a file specified
+ * by path when called by non-root user with the following constraints,
+ * - euid of the process is equal to the owner of the file.
+ * - the intended gid is either egid, or one of the supplementary gids
+ *   of the process.
+ * Also, verify that chown() clears the setuid/setgid bits set on the file.
  */

 #include <stdio.h>
@@ -80,123 +26,76 @@
 #include <grp.h>
 #include <pwd.h>

-#include "test.h"
-#include "safe_macros.h"
-#include "compat_16.h"
-
-#define FILE_MODE	(S_IFREG|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define NEW_PERMS	(S_IFREG|S_IRWXU|S_IRWXG|S_ISUID|S_ISGID)
-#define TESTFILE	"testfile"
+#include "tst_test.h"
+#include "compat_tst_16.h"

-TCID_DEFINE(chown03);
-int TST_TOTAL = 1;		/* Total number of test conditions */
-char nobody_uid[] = "nobody";
-struct passwd *ltpuser;
+#define FILE_MODE (S_IFREG|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
+#define NEW_PERMS (S_IFREG|S_IRWXU|S_IRWXG|S_ISUID|S_ISGID)
+#define FILENAME "chown03_testfile"

-void setup();			/* setup function for the test */
-void cleanup();			/* cleanup function for the test */
+static struct passwd *ltpuser;

-int main(int ac, char **av)
+static void check_owner(struct stat *s, uid_t exp_uid, gid_t exp_gid)
 {
-	struct stat stat_buf;	/* stat(2) struct contents */
-	int lc;
-	uid_t user_id;		/* Owner id of the test file. */
-	gid_t group_id;		/* Group id of the test file. */
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		UID16_CHECK((user_id = geteuid()), "chown", cleanup)
-		GID16_CHECK((group_id = getegid()), "chown", cleanup)
-
-		TEST(CHOWN(cleanup, TESTFILE, -1, group_id));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL | TTERRNO, "chown(%s, ..) failed",
-				 TESTFILE);
-			continue;
-		}
-
-		if (stat(TESTFILE, &stat_buf) == -1)
-			tst_brkm(TFAIL | TERRNO, cleanup,
-				 "stat failed");
-
-		if (stat_buf.st_uid != user_id ||
-		    stat_buf.st_gid != group_id)
-			tst_resm(TFAIL, "%s: Incorrect ownership"
-				 "set to %d %d, Expected %d %d",
-				 TESTFILE, stat_buf.st_uid,
-				 stat_buf.st_gid, user_id, group_id);
-
-		if (stat_buf.st_mode !=
-		    (NEW_PERMS & ~(S_ISUID | S_ISGID)))
-			tst_resm(TFAIL, "%s: incorrect mode permissions"
-				 " %#o, Expected %#o", TESTFILE,
-				 stat_buf.st_mode,
-				 NEW_PERMS & ~(S_ISUID | S_ISGID));
-		else
-			tst_resm(TPASS, "chown(%s, ..) was successful",
-				 TESTFILE);
-	}
-
-	cleanup();
-	tst_exit();
+	if (s->st_uid != exp_uid || s->st_gid != exp_gid)
+		tst_res(TFAIL, "%s: wrong owner set to (uid=%d, gid=%d),"
+			       " expected (uid=%d, gid=%d)",
+			FILENAME, s->st_uid, s->st_gid, exp_uid, exp_gid);
+	else
+		tst_res(TPASS, "%s: expected owner set to (uid=%d, gid=%d)",
+			FILENAME, exp_uid, exp_gid);
 }

-/*
- * void
- * setup() - performs all ONE TIME setup for this test.
- *  Create a temporary directory and change directory to it.
- *  Create a test file under temporary directory and close it
- *  Change the group ownership on testfile.
- */
-void setup(void)
+static void run(void)
 {
-	int fd;			/* file handler for testfile */
-
-	TEST_PAUSE;
-
-	tst_require_root();
-
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-
-	tst_tmpdir();
-
-	ltpuser = getpwnam(nobody_uid);
-	if (ltpuser == NULL)
-		tst_brkm(TBROK | TERRNO, NULL, "getpwnam(\"nobody\") failed");
-	SAFE_SETEGID(NULL, ltpuser->pw_gid);
-	SAFE_SETEUID(NULL, ltpuser->pw_uid);
-
-	/* Create a test file under temporary directory */
-	if ((fd = open(TESTFILE, O_RDWR | O_CREAT, FILE_MODE)) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "open(%s, O_RDWR|O_CREAT, %o) failed", TESTFILE,
-			 FILE_MODE);
-
-	SAFE_SETEUID(cleanup, 0);
-
-	SAFE_FCHOWN(cleanup, fd, -1, 0);
+	SAFE_SETEUID(0);
+	SAFE_CHOWN(FILENAME, -1, 0);
+	SAFE_CHMOD(FILENAME, NEW_PERMS);
+	SAFE_SETEUID(ltpuser->pw_uid);
+
+	uid_t uid;
+	gid_t gid;
+	UID16_CHECK((uid = geteuid()), "chown");
+	GID16_CHECK((gid = getegid()), "chown");
+
+	struct stat stat_buf;
+	SAFE_STAT(FILENAME, &stat_buf);
+	check_owner(&stat_buf, uid, 0);
+
+	TST_EXP_PASS(CHOWN(FILENAME, -1, gid), "chown(%s, %d, %d)",
+		     FILENAME, -1, gid);
+	SAFE_STAT(FILENAME, &stat_buf);
+	check_owner(&stat_buf, uid, gid);
+
+	if (stat_buf.st_mode != (NEW_PERMS & ~(S_ISUID | S_ISGID)))
+		tst_res(TFAIL, "%s: incorrect mode permissions %#o, expected %#o",
+			FILENAME, stat_buf.st_mode, NEW_PERMS & ~(S_ISUID | S_ISGID));
+	else
+		tst_res(TPASS, "chown(%s, ..)", FILENAME);
+}

-	SAFE_FCHMOD(cleanup, fd, NEW_PERMS);
+static void setup(void)
+{
+	int fd;

-	SAFE_SETEUID(cleanup, ltpuser->pw_uid);
+	ltpuser = SAFE_GETPWNAM("nobody");
+	SAFE_SETEGID(ltpuser->pw_gid);
+	SAFE_SETEUID(ltpuser->pw_uid);

-	SAFE_CLOSE(cleanup, fd);
+	fd = SAFE_OPEN(FILENAME, O_RDWR | O_CREAT, FILE_MODE);
+	SAFE_CLOSE(fd);
 }

-void cleanup(void)
+static void cleanup(void)
 {
-	if (setegid(0) == -1)
-		tst_resm(TWARN | TERRNO, "setegid(0) failed");
-	if (seteuid(0) == -1)
-		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
-
-	tst_rmdir();
-
+	SAFE_SETEGID(0);
+	SAFE_SETEUID(0);
 }
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run,
+};
--
2.17.1

[LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

[xieziyao]

Hi,

Thanks so much for your review!

I just re-checked the latest code and submit the v4 version based on your suggestions:
1. Move two similar owner checks to a single function;
2. Modify the incorrect code style.

Please see: https://patchwork.ozlabs.org/project/ltp/patch/20210420061954.155049-1-xieziyao@huawei.com/

Thanks very much!

Best Regards,
Ziyao

-----Original Message-----
From: Alexey Kodanev [mailto:alexey.kodanev@oracle.com] 
Sent: Tuesday, April 20, 2021 12:09 AM
To: xieziyao <xieziyao@huawei.com>; ltp@lists.linux.it
Subject: Re: [LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

On 19.04.2021 04:34, Xie Ziyao wrote:
> For this:
>   testcases/kernel/syscalls/chown/chown03.c
> 
> Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
> ---
> v2->v3:
> 1. Remove some unnecessary comments and incorrect output prints; 2. 
> Moved some prerequisite codes from the setup() function to the run() 
> function and add code logic for checking whether the setting is successful.
> 

Hi Ziyao,

>  testcases/kernel/syscalls/chown/chown03.c | 241 
> +++++++---------------
>  1 file changed, 70 insertions(+), 171 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/chown/chown03.c 
> b/testcases/kernel/syscalls/chown/chown03.c
> index 2c7bcfe7d..4469f1c4d 100644
> --- a/testcases/kernel/syscalls/chown/chown03.c
> +++ b/testcases/kernel/syscalls/chown/chown03.c
> @@ -1,72 +1,18 @@

...
> -int TST_TOTAL = 1;		/* Total number of test conditions */
> -char nobody_uid[] = "nobody";
>  struct passwd *ltpuser;

static struct passwd *ltpuser;

> 
> -void setup();			/* setup function for the test */
> -void cleanup();			/* cleanup function for the test */
> -
> -int main(int ac, char **av)
> +static void run(void)
>  {
...
> +    SAFE_SETEUID(0);
> +    SAFE_CHOWN(FILENAME, -1, 0);
> +    SAFE_CHMOD(FILENAME, NEW_PERMS);
> +    SAFE_SETEUID(ltpuser->pw_uid);
> +
> +    uid_t uid;
> +    gid_t gid;
> +    UID16_CHECK((uid = geteuid()), "chown");
> +    GID16_CHECK((gid = getegid()), "chown");
> +
> +    struct stat stat_buf;
> +    SAFE_STAT(FILENAME, &stat_buf);
> +    if (stat_buf.st_uid != uid || stat_buf.st_gid != 0)
> +        tst_res(TFAIL, "%s: Incorrect ownership"
> +                       "set to %d %d, Expected %d %d",
> +                FILENAME, stat_buf.st_uid,
> +                stat_buf.st_gid, uid, 0);
> +
> +    TST_EXP_PASS(CHOWN(FILENAME, -1, gid), "chown(%s,%d,%d)",
> +                 FILENAME, -1, gid);
> +
> +    SAFE_STAT(FILENAME, &stat_buf);
> +    if (stat_buf.st_uid != uid || stat_buf.st_gid != gid)
> +        tst_res(TFAIL, "%s: Incorrect ownership"
> +                       "set to %d %d, Expected %d %d",
> +                FILENAME, stat_buf.st_uid,
> +                stat_buf.st_gid, uid, gid);

There are two similar owner checks now, it would be better to move them to a single function, something like this:

static void check_owner(struct stat *s, uid_t exp_uid, gid_t exp_gid) {
        if (s->st_uid != exp_uid || s->st_gid != exp_gid) {
                tst_res(TFAIL, "%s: wrong owner set to %d %d, expected %d %d",
                        FILENAME, s->st_uid, s->st_gid, exp_uid, exp_gid);
        } else {
                tst_res(TPASS, "%s: expected owner set %d %d",
                        FILENAME, exp_uid, exp_gid);
        }
}

[LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

[xieziyao]

Hi,

Thanks so much for your review!

I just re-checked the latest code and submit the v4 version based on your suggestions:
1. Move two similar owner checks to a single function;
2. Modify the incorrect code style.

Please see: https://patchwork.ozlabs.org/project/ltp/patch/20210420061954.155049-1-xieziyao@huawei.com/

Thanks very much!

Best Regards,
Ziyao

-----Original Message-----
From: Cyril Hrubis [mailto:chrubis@suse.cz] 
Sent: Monday, April 19, 2021 10:56 PM
To: xieziyao <xieziyao@huawei.com>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH v3] syscalls/chown: Rewrite chown/chown03.c with the new api

Hi!
> +    SAFE_SETEUID(0);
> +    SAFE_CHOWN(FILENAME, -1, 0);
> +    SAFE_CHMOD(FILENAME, NEW_PERMS);
> +    SAFE_SETEUID(ltpuser->pw_uid);

LTP uses Linux kernel conding style which requires tabs for indentation, while this code uses four spaces.

You can use checkpatch.pl shipped along with Linux kernel sources to check coding style before sending patches to the list.

--
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4] syscalls/chown: Rewrite chown/chown03.c with the new api

[Cyril Hrubis]

Hi!
> +	SAFE_SETEUID(0);
> +	SAFE_CHOWN(FILENAME, -1, 0);
> +	SAFE_CHMOD(FILENAME, NEW_PERMS);
> +	SAFE_SETEUID(ltpuser->pw_uid);
> +
> +	uid_t uid;
> +	gid_t gid;
> +	UID16_CHECK((uid = geteuid()), "chown");
> +	GID16_CHECK((gid = getegid()), "chown");
> +
> +	struct stat stat_buf;
> +	SAFE_STAT(FILENAME, &stat_buf);
> +	check_owner(&stat_buf, uid, 0);

We should check that the S_ISUID and S_ISGID bits are set here as well.

Otherwise it looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4] syscalls/chown: Rewrite chown/chown03.c with the new api

[xieziyao]

Hi,

Thanks, I just made changes on your suggestions.

Please see: https://patchwork.ozlabs.org/project/ltp/patch/20210422024410.175538-1-xieziyao@huawei.com/

Best Regards,
Ziyao

-----Original Message-----
From: Cyril Hrubis [mailto:chrubis@suse.cz] 
Sent: Wednesday, April 21, 2021 7:58 PM
To: xieziyao <xieziyao@huawei.com>
Cc: ltp@lists.linux.it; alexey.kodanev@oracle.com
Subject: Re: [LTP][PATCH v4] syscalls/chown: Rewrite chown/chown03.c with the new api

Hi!
> +	SAFE_SETEUID(0);
> +	SAFE_CHOWN(FILENAME, -1, 0);
> +	SAFE_CHMOD(FILENAME, NEW_PERMS);
> +	SAFE_SETEUID(ltpuser->pw_uid);
> +
> +	uid_t uid;
> +	gid_t gid;
> +	UID16_CHECK((uid = geteuid()), "chown");
> +	GID16_CHECK((gid = getegid()), "chown");
> +
> +	struct stat stat_buf;
> +	SAFE_STAT(FILENAME, &stat_buf);
> +	check_owner(&stat_buf, uid, 0);

We should check that the S_ISUID and S_ISGID bits are set here as well.

Otherwise it looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz