Re: [LTP] [PATCH 2/2] fchownat/fchownat02: Convert to new API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
Same comment apply here as well, 'make check' produces many warnings
that should have been fixed.

> +/*\
> + * [Description]
> + *
> + * Verify that,
> + * The flag of fchownat() is AT_SYMLINK_NOFOLLOW and the pathname would
> + * not be dereferenced if the pathname is a symbolic link.

This is a bit broken, at least the sentence should continue with
lowercase The on the second line as continuation of the sentence that
has started with 'Verify that,'.

But it would be better rewritten to something as:

Verify that fchownat() with AT_SYMLINK_NOFOLLOW does not dereference
symbolic links.

>   */
> 
>  #define _GNU_SOURCE
> 
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <fcntl.h>
> -#include <unistd.h>
> -#include <stdlib.h>
> -#include <errno.h>
> -#include <string.h>
> -#include <signal.h>
> -#include "test.h"
> -#include "safe_macros.h"
> +#include "tst_test.h"
>  #include "fchownat.h"
>  #include "lapi/fcntl.h"
> 
>  #define TESTFILE	"testfile"
>  #define TESTFILE_LINK	"testfile_link"
> 
> -char *TCID = "fchownat02";
> -int TST_TOTAL = 1;
> -
>  static int dir_fd;
>  static uid_t set_uid = 1000;
>  static gid_t set_gid = 1000;
> -static void setup(void);
> -static void cleanup(void);
> -static void test_verify(void);
> -static void fchownat_verify(void);
> -
> -int main(int ac, char **av)
> -{
> -	int lc;
> -	int i;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -		for (i = 0; i < TST_TOTAL; i++)
> -			fchownat_verify();
> -	}
> -
> -	cleanup();
> -	tst_exit();
> -}
> 
>  static void setup(void)
>  {
>  	struct stat c_buf, l_buf;
> 
> -	if ((tst_kvercmp(2, 6, 16)) < 0)
> -		tst_brkm(TCONF, NULL, "This test needs kernel 2.6.16 or newer");
> -
> -	tst_require_root();
> +	dir_fd = SAFE_OPEN("./", O_DIRECTORY);
> 
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> +	SAFE_TOUCH(TESTFILE, 0600, NULL);
> 
> -	TEST_PAUSE;
> +	SAFE_SYMLINK(TESTFILE, TESTFILE_LINK);
> 
> -	tst_tmpdir();
> +	SAFE_STAT(TESTFILE_LINK, &c_buf);
> 
> -	dir_fd = SAFE_OPEN(cleanup, "./", O_DIRECTORY);
> -
> -	SAFE_TOUCH(cleanup, TESTFILE, 0600, NULL);
> -
> -	SAFE_SYMLINK(cleanup, TESTFILE, TESTFILE_LINK);
> -
> -	SAFE_STAT(cleanup, TESTFILE_LINK, &c_buf);
> -
> -	SAFE_LSTAT(cleanup, TESTFILE_LINK, &l_buf);
> +	SAFE_LSTAT(TESTFILE_LINK, &l_buf);
> 
>  	if (l_buf.st_uid == set_uid || l_buf.st_gid == set_gid) {
> -		tst_brkm(TBROK | TERRNO, cleanup,
> +		tst_brk(TBROK | TERRNO,
>  			 "link_uid(%d) == set_uid(%d) or link_gid(%d) == "
>  			 "set_gid(%d)", l_buf.st_uid, set_uid, l_buf.st_gid,
>  			 set_gid);
>  	}

I guess that it would be easier to set the set_uid and set_gid to
l_buf.st_uid + 1 and l_buf.st_gid + 1 that way we will be sure that it's
different.

>  }
> 
> -static void fchownat_verify(void)
> -{
> -	TEST(fchownat(dir_fd, TESTFILE_LINK, set_uid, set_gid,
> -		      AT_SYMLINK_NOFOLLOW));
> -
> -	if (TEST_RETURN != 0) {
> -		tst_resm(TFAIL | TTERRNO, "fchownat() failed, errno=%d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> -	} else {
> -		test_verify();
> -	}
> -}
> -
> -static void test_verify(void)
> +static void verify_fchownat(void)
>  {
>  	struct stat c_buf, l_buf;
> 
> -	SAFE_STAT(cleanup, TESTFILE_LINK, &c_buf);
> +	TST_EXP_PASS_SILENT(fchownat(dir_fd, TESTFILE_LINK, set_uid, set_gid,
> +		      AT_SYMLINK_NOFOLLOW));
> +
> +	SAFE_STAT(TESTFILE_LINK, &c_buf);
> 
> -	SAFE_LSTAT(cleanup, TESTFILE_LINK, &l_buf);
> +	SAFE_LSTAT(TESTFILE_LINK, &l_buf);
> 
>  	if (c_buf.st_uid != set_uid && l_buf.st_uid == set_uid &&
>  	    c_buf.st_gid != set_gid && l_buf.st_gid == set_gid) {
> -		tst_resm(TPASS, "fchownat() test AT_SYMLINK_NOFOLLOW success");
> +		tst_res(TPASS, "fchownat() test AT_SYMLINK_NOFOLLOW success");
>  	} else {
> -		tst_resm(TFAIL,
> +		tst_res(TFAIL,
>  			 "fchownat() test AT_SYMLINK_NOFOLLOW fail with uid=%d "
>  			 "link_uid=%d set_uid=%d | gid=%d link_gid=%d "
>  			 "set_gid=%d", c_buf.st_uid, l_buf.st_uid, set_uid,
> @@ -136,5 +73,15 @@ static void test_verify(void)
> 
>  static void cleanup(void)
>  {
> -	tst_rmdir();
> +       if (dir_fd > 0)
> +               close(dir_fd);

Here SAFE_CLOSE() as well.

>  }
> +
> +static struct tst_test test = {
> +        .min_kver = "2.6.16",
> +        .test_all = verify_fchownat,
> +        .setup = setup,
> +        .cleanup = cleanup,
> +        .needs_tmpdir = 1,
> +	.needs_root = 1,
> +};
> --
> 2.20.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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