[LTP] [PATCH v3 1/1] setgroups03: Fix running more iterations (-i 2)

[LTP] [PATCH v3 1/1] setgroups03: Fix running more iterations (-i 2)

[Petr Vorel]

From: Zhao Gongyi <zhaogongyi@huawei.com>

When run the test with option "-i 2", test will fail and
report "setgroups03.c:157: setgroups(65537) fails, Size
is > sysconf(_SC_NGROUPS_MAX), errno=1, expected errno=22".

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
[ pvorel: Add parameters to setup1() to use single function, use
SAFE_GETPWNAM() ]
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v2->v3.
* move ltpuser = SAFE_GETPWNAM(cleanup, uid); to setup() (Cyril)
* add second parameter uid_t euid to setup1() (Cyril)

Kind regards,
Petr

 .../kernel/syscalls/setgroups/setgroups03.c   | 38 ++++++++-----------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/testcases/kernel/syscalls/setgroups/setgroups03.c b/testcases/kernel/syscalls/setgroups/setgroups03.c
index 490b06996..21894d5af 100644
--- a/testcases/kernel/syscalls/setgroups/setgroups03.c
+++ b/testcases/kernel/syscalls/setgroups/setgroups03.c
@@ -73,11 +73,9 @@
 #include <grp.h>
 
 #include "test.h"
-
+#include "safe_macros.h"
 #include "compat_16.h"
 
-#define TESTUSER	"nobody"
-
 char nobody_uid[] = "nobody";
 struct passwd *ltpuser;
 
@@ -86,7 +84,7 @@ int TST_TOTAL = 2;
 
 GID_T *groups_list;		/* Array to hold gids for getgroups() */
 
-int setup1();			/* setup function to test error EPERM */
+void setup1(const char *uid, uid_t euid);	/* setup function to test error EPERM */
 void setup();			/* setup function for the test */
 void cleanup();			/* cleanup function for the test */
 
@@ -95,7 +93,7 @@ struct test_case_t {		/* test case struct. to hold ref. test cond's */
 	int list;
 	char *desc;
 	int exp_errno;
-	int (*setupfunc) ();
+	void (*setupfunc)(const char *uid, uid_t euid);
 } Test_cases[] = {
 	{
 	1, 1, "Size is > sysconf(_SC_NGROUPS_MAX)", EINVAL, NULL}, {
@@ -126,7 +124,7 @@ int main(int ac, char **av)
 
 		for (i = 0; i < TST_TOTAL; i++) {
 			if (Test_cases[i].setupfunc != NULL) {
-				Test_cases[i].setupfunc();
+				Test_cases[i].setupfunc(nobody_uid, ltpuser->pw_uid);
 			}
 
 			gidsetsize = ngroups_max + Test_cases[i].gsize_add;
@@ -156,8 +154,11 @@ int main(int ac, char **av)
 					 gidsetsize, test_desc, TEST_ERRNO,
 					 Test_cases[i].exp_errno);
 			}
-		}
 
+			if (Test_cases[i].setupfunc != NULL) {
+				Test_cases[i].setupfunc("root", ltpuser->pw_uid);
+			}
+		}
 	}
 
 	cleanup();
@@ -176,8 +177,9 @@ void setup(void)
 
 	tst_sig(NOFORK, DEF_HANDLER, cleanup);
 
-	TEST_PAUSE;
+	ltpuser = SAFE_GETPWNAM(cleanup, uid);
 
+	TEST_PAUSE;
 }
 
 /*
@@ -187,29 +189,21 @@ void setup(void)
  *  Get the user info. from /etc/passwd file.
  *  This function returns 0 on success.
  */
-int setup1(void)
+void setup1(const char *uid, uid_t euid)
 {
-	struct passwd *user_info;	/* struct. to hold test user info */
-
-/* Switch to nobody user for correct error code collection */
-	ltpuser = getpwnam(nobody_uid);
-	if (seteuid(ltpuser->pw_uid) == -1) {
-		tst_resm(TINFO, "setreuid failed to "
-			 "to set the effective uid to %d", ltpuser->pw_uid);
-		perror("setreuid");
-	}
+	struct passwd *user_info;
 
-	if ((user_info = getpwnam(TESTUSER)) == NULL) {
-		tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed", TESTUSER);
-	}
+	SAFE_SETEUID(cleanup, euid);
+
+	user_info = SAFE_GETPWNAM(cleanup, uid);
 
 	if (!GID_SIZE_CHECK(user_info->pw_gid)) {
 		tst_brkm(TBROK,
 			 cleanup,
 			 "gid returned from getpwnam is too large for testing setgroups16");
 	}
+
 	groups_list[0] = user_info->pw_gid;
-	return 0;
 }
 
 /*
-- 
2.33.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/1] setgroups03: Fix running more iterations (-i 2)

[Cyril Hrubis]

Hi!
> -int setup1();			/* setup function to test error EPERM */
> +void setup1(const char *uid, uid_t euid);	/* setup function to test error EPERM */
>  void setup();			/* setup function for the test */
>  void cleanup();			/* cleanup function for the test */
>  
> @@ -95,7 +93,7 @@ struct test_case_t {		/* test case struct. to hold ref. test cond's */
>  	int list;
>  	char *desc;
>  	int exp_errno;
> -	int (*setupfunc) ();
> +	void (*setupfunc)(const char *uid, uid_t euid);
>  } Test_cases[] = {
>  	{
>  	1, 1, "Size is > sysconf(_SC_NGROUPS_MAX)", EINVAL, NULL}, {
> @@ -126,7 +124,7 @@ int main(int ac, char **av)
>  
>  		for (i = 0; i < TST_TOTAL; i++) {
>  			if (Test_cases[i].setupfunc != NULL) {
> -				Test_cases[i].setupfunc();
> +				Test_cases[i].setupfunc(nobody_uid, ltpuser->pw_uid);
>  			}
>  
>  			gidsetsize = ngroups_max + Test_cases[i].gsize_add;
> @@ -156,8 +154,11 @@ int main(int ac, char **av)
>  					 gidsetsize, test_desc, TEST_ERRNO,
>  					 Test_cases[i].exp_errno);
>  			}
> -		}
>  
> +			if (Test_cases[i].setupfunc != NULL) {
> +				Test_cases[i].setupfunc("root", ltpuser->pw_uid);
> +			}
> +		}
>  	}
>  
>  	cleanup();
> @@ -176,8 +177,9 @@ void setup(void)
>  
>  	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>  
> -	TEST_PAUSE;
> +	ltpuser = SAFE_GETPWNAM(cleanup, uid);
>  
> +	TEST_PAUSE;
>  }
>  
>  /*
> @@ -187,29 +189,21 @@ void setup(void)
>   *  Get the user info. from /etc/passwd file.
>   *  This function returns 0 on success.
>   */
> -int setup1(void)
> +void setup1(const char *uid, uid_t euid)
>  {
> -	struct passwd *user_info;	/* struct. to hold test user info */
> -
> -/* Switch to nobody user for correct error code collection */
> -	ltpuser = getpwnam(nobody_uid);
> -	if (seteuid(ltpuser->pw_uid) == -1) {
> -		tst_resm(TINFO, "setreuid failed to "
> -			 "to set the effective uid to %d", ltpuser->pw_uid);
> -		perror("setreuid");
> -	}
> +	struct passwd *user_info;
>  
> -	if ((user_info = getpwnam(TESTUSER)) == NULL) {
> -		tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed", TESTUSER);
> -	}
> +	SAFE_SETEUID(cleanup, euid);
> +
> +	user_info = SAFE_GETPWNAM(cleanup, uid);

I still do not get why we call SAFE_GETPWNAM() here. We should do that
in the setup and prepare two different group_list[] lists, if that is
really needed.

But I guess that all we need in this test is:

* Run the EINVAL test as a root

* Run the EPERM test as a nobody

The content of the list should not matter, as a matter of a fact we pass
unitialized data in the EINVAL case. What matters is the size argument,
it should be 1 for the EPERM test and max+1 for the EINVAL case.

>  	if (!GID_SIZE_CHECK(user_info->pw_gid)) {
>  		tst_brkm(TBROK,
>  			 cleanup,
>  			 "gid returned from getpwnam is too large for testing setgroups16");
>  	}
> +
>  	groups_list[0] = user_info->pw_gid;
> -	return 0;
>  }
>  
>  /*
> -- 
> 2.33.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/1] setgroups03: Fix running more iterations (-i 2)

[Petr Vorel]

Hi Cyril, Zhao,

> Hi!
> > -int setup1();			/* setup function to test error EPERM */
> > +void setup1(const char *uid, uid_t euid);	/* setup function to test error EPERM */
> >  void setup();			/* setup function for the test */
> >  void cleanup();			/* cleanup function for the test */

> > @@ -95,7 +93,7 @@ struct test_case_t {		/* test case struct. to hold ref. test cond's */
> >  	int list;
> >  	char *desc;
> >  	int exp_errno;
> > -	int (*setupfunc) ();
> > +	void (*setupfunc)(const char *uid, uid_t euid);
> >  } Test_cases[] = {
> >  	{
> >  	1, 1, "Size is > sysconf(_SC_NGROUPS_MAX)", EINVAL, NULL}, {
> > @@ -126,7 +124,7 @@ int main(int ac, char **av)

> >  		for (i = 0; i < TST_TOTAL; i++) {
> >  			if (Test_cases[i].setupfunc != NULL) {
> > -				Test_cases[i].setupfunc();
> > +				Test_cases[i].setupfunc(nobody_uid, ltpuser->pw_uid);
> >  			}

> >  			gidsetsize = ngroups_max + Test_cases[i].gsize_add;
> > @@ -156,8 +154,11 @@ int main(int ac, char **av)
> >  					 gidsetsize, test_desc, TEST_ERRNO,
> >  					 Test_cases[i].exp_errno);
> >  			}
> > -		}

> > +			if (Test_cases[i].setupfunc != NULL) {
> > +				Test_cases[i].setupfunc("root", ltpuser->pw_uid);
> > +			}
> > +		}
> >  	}

> >  	cleanup();
> > @@ -176,8 +177,9 @@ void setup(void)

> >  	tst_sig(NOFORK, DEF_HANDLER, cleanup);

> > -	TEST_PAUSE;
> > +	ltpuser = SAFE_GETPWNAM(cleanup, uid);

> > +	TEST_PAUSE;
> >  }

> >  /*
> > @@ -187,29 +189,21 @@ void setup(void)
> >   *  Get the user info. from /etc/passwd file.
> >   *  This function returns 0 on success.
> >   */
> > -int setup1(void)
> > +void setup1(const char *uid, uid_t euid)
> >  {
> > -	struct passwd *user_info;	/* struct. to hold test user info */
> > -
> > -/* Switch to nobody user for correct error code collection */
> > -	ltpuser = getpwnam(nobody_uid);
> > -	if (seteuid(ltpuser->pw_uid) == -1) {
> > -		tst_resm(TINFO, "setreuid failed to "
> > -			 "to set the effective uid to %d", ltpuser->pw_uid);
> > -		perror("setreuid");
> > -	}
> > +	struct passwd *user_info;

> > -	if ((user_info = getpwnam(TESTUSER)) == NULL) {
> > -		tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed", TESTUSER);
> > -	}
> > +	SAFE_SETEUID(cleanup, euid);
> > +
> > +	user_info = SAFE_GETPWNAM(cleanup, uid);

> I still do not get why we call SAFE_GETPWNAM() here. We should do that
> in the setup and prepare two different group_list[] lists, if that is
> really needed.

> But I guess that all we need in this test is:

> * Run the EINVAL test as a root

> * Run the EPERM test as a nobody

> The content of the list should not matter, as a matter of a fact we pass
> unitialized data in the EINVAL case. What matters is the size argument,
> it should be 1 for the EPERM test and max+1 for the EINVAL case.

Good point, thank you!

@Zhao feel free to let me know you're doing to implement it.
Otherwise I'll have look on Monday.

Kind regards,
Petr

> >  	if (!GID_SIZE_CHECK(user_info->pw_gid)) {
> >  		tst_brkm(TBROK,
> >  			 cleanup,
> >  			 "gid returned from getpwnam is too large for testing setgroups16");
> >  	}
> > +
> >  	groups_list[0] = user_info->pw_gid;
> > -	return 0;
> >  }

> >  /*
> > -- 
> > 2.33.0

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/1] setgroups03: Fix running more iterations (-i 2)

[zhaogongyi]

Hi,

For the testcase setgroup3:

	* Test Description:
 	*  Verify that,
 	*   1. setgroups() fails with -1 and sets errno to EINVAL if the size
 	*      argument value is > NGROUPS
 	*   2. setgroups() fails with -1 and sets errno to EPERM if the
 	*      calling process is not super-user.

At the first checkpoint, set errno to EINVAL if the size argument value is > NGROUPS, it seems that has no matter with the value of group_list. Meticulously, we can set group_list to a normal value.

Same situation for the second checkpoint.

So can we reserve the group_list that set a normal value?


Thanks so much!


> 
> Hi,
> 
> Yes, in this testcase, the groups_list is redundant and can be removed.
> 
> I am sorry for my late reply.
> 
> Thanks so much!
> 
> 
> 
> > > > -int setup1(void)
> > > > +void setup1(const char *uid, uid_t euid)
> > > >  {
> > > > -	struct passwd *user_info;	/* struct. to hold test user info */
> > > > -
> > > > -/* Switch to nobody user for correct error code collection */
> > > > -	ltpuser = getpwnam(nobody_uid);
> > > > -	if (seteuid(ltpuser->pw_uid) == -1) {
> > > > -		tst_resm(TINFO, "setreuid failed to "
> > > > -			 "to set the effective uid to %d", ltpuser->pw_uid);
> > > > -		perror("setreuid");
> > > > -	}
> > > > +	struct passwd *user_info;
> >
> > > > -	if ((user_info = getpwnam(TESTUSER)) == NULL) {
> > > > -		tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed",
> > TESTUSER);
> > > > -	}
> > > > +	SAFE_SETEUID(cleanup, euid);
> > > > +
> > > > +	user_info = SAFE_GETPWNAM(cleanup, uid);
> >
> > > I still do not get why we call SAFE_GETPWNAM() here. We should do
> > > that in the setup and prepare two different group_list[] lists, if
> > > that is really needed.
> >
> > > But I guess that all we need in this test is:
> >
> > > * Run the EINVAL test as a root
> >
> > > * Run the EPERM test as a nobody
> >
> > > The content of the list should not matter, as a matter of a fact we
> > > pass unitialized data in the EINVAL case. What matters is the size
> > > argument, it should be 1 for the EPERM test and max+1 for the EINVAL
> > case.
> >
> > Good point, thank you!
> >
> > @Zhao feel free to let me know you're doing to implement it.
> > Otherwise I'll have look on Monday.
> >
> > Kind regards,
> > Petr
> >
> > > >  	if (!GID_SIZE_CHECK(user_info->pw_gid)) {
> > > >  		tst_brkm(TBROK,
> > > >  			 cleanup,
> > > >  			 "gid returned from getpwnam is too large for testing
> > setgroups16");
> > > >  	}
> > > > +
> > > >  	groups_list[0] = user_info->pw_gid;
> > > > -	return 0;
> > > >  }
> >
> > > >  /*
> > > > --
> > > > 2.33.0

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/1] setgroups03: Fix running more iterations (-i 2)

[zhaogongyi]

Hi,

Yes, in this testcase, the groups_list is redundant and can be removed.

I am sorry for my late reply.

Thanks so much!



> > > -int setup1(void)
> > > +void setup1(const char *uid, uid_t euid)
> > >  {
> > > -	struct passwd *user_info;	/* struct. to hold test user info */
> > > -
> > > -/* Switch to nobody user for correct error code collection */
> > > -	ltpuser = getpwnam(nobody_uid);
> > > -	if (seteuid(ltpuser->pw_uid) == -1) {
> > > -		tst_resm(TINFO, "setreuid failed to "
> > > -			 "to set the effective uid to %d", ltpuser->pw_uid);
> > > -		perror("setreuid");
> > > -	}
> > > +	struct passwd *user_info;
> 
> > > -	if ((user_info = getpwnam(TESTUSER)) == NULL) {
> > > -		tst_brkm(TFAIL, cleanup, "getpwnam(2) of %s Failed",
> TESTUSER);
> > > -	}
> > > +	SAFE_SETEUID(cleanup, euid);
> > > +
> > > +	user_info = SAFE_GETPWNAM(cleanup, uid);
> 
> > I still do not get why we call SAFE_GETPWNAM() here. We should do that
> > in the setup and prepare two different group_list[] lists, if that is
> > really needed.
> 
> > But I guess that all we need in this test is:
> 
> > * Run the EINVAL test as a root
> 
> > * Run the EPERM test as a nobody
> 
> > The content of the list should not matter, as a matter of a fact we
> > pass unitialized data in the EINVAL case. What matters is the size
> > argument, it should be 1 for the EPERM test and max+1 for the EINVAL
> case.
> 
> Good point, thank you!
> 
> @Zhao feel free to let me know you're doing to implement it.
> Otherwise I'll have look on Monday.
> 
> Kind regards,
> Petr
> 
> > >  	if (!GID_SIZE_CHECK(user_info->pw_gid)) {
> > >  		tst_brkm(TBROK,
> > >  			 cleanup,
> > >  			 "gid returned from getpwnam is too large for testing
> setgroups16");
> > >  	}
> > > +
> > >  	groups_list[0] = user_info->pw_gid;
> > > -	return 0;
> > >  }
> 
> > >  /*
> > > --
> > > 2.33.0

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp