[LTP] [PATCH] acct01: add EFAULT errno check.

[路斐]

Hi Petr, Cyril.
Really thanks for the advises. Each of them makes me learned.


Sorry for the patches not so good I've sent, I'm trying to make it better in the future.
I will make warnings clearnup patches after this one merged.


Best reguards.
Lu Fei
 
------------------ Original ------------------
From:  "Petr Vorel"<pvorel@suse.cz>;
Date:  Mon, Jul 8, 2024 04:23 AM
To:  "ltp"<ltp@lists.linux.it>; "lufei"<lufei@uniontech.com>; 
Cc:  "Cyril Hrubis"<chrubis@suse.cz>; 
Subject:  Re: [LTP] [PATCH] acct01: add EFAULT errno check.

 

Hi 
> Add EFAULT errno check in acct01 testcase.

> Signed-off-by: lufei <lufei@uniontech.com>

I guess you don't mind if I change this to following before merge:
Signed-off-by: Lu Fei <lufei@uniontech.com>

> ---
>  testcases/kernel/syscalls/acct/acct01.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

> diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
> index 1b53a32f2..ed1817bc5 100644
> --- a/testcases/kernel/syscalls/acct/acct01.c
> +++ b/testcases/kernel/syscalls/acct/acct01.c
> @@ -33,6 +33,7 @@
>  #define FILE_TMPFILE		"./tmpfile"
>  #define FILE_ELOOP		"test_file_eloop1"
>  #define FILE_EROFS		"ro_mntpoint/file"
> +#define FILE_EFAULT		"/tmp/invalid/file/name"

And here I just amend to:
#define FILE_EFAULT            "invalid/file/name"

(although it's very unlikely, the full patch *could* be existing, but not the
relative one, because LTP is creating unique temporary directory for each run,
e.g. /tmp/LTP_accTJpYqc.

>  static struct passwd *ltpuser;

> @@ -45,6 +46,7 @@ static char *file_eloop;
>  static char *file_enametoolong;
>  static char *file_erofs;
>  static char *file_null;
> +static char *file_efault;

>  static void setup_euid(void)
>  {
> @@ -56,6 +58,16 @@ static void cleanup_euid(void)
>  	SAFE_SETEUID(0);
>  }

> +static void setup_emem(void)
> +{
> +	file_efault = SAFE_MMAP(NULL, 1, PROT_NONE,
> +			MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> +}
> +static void cleanup_emem(void)
> +{
> +	SAFE_MUNMAP(file_efault, 1);
> +}
> +
>  static struct test_case {
>  	char **filename;
>  	char *desc;
> @@ -72,6 +84,7 @@ static struct test_case {
>  	{&file_eloop,   FILE_ELOOP,   ELOOP,        NULL, NULL},
>  	{&file_enametoolong, "aaaa...", ENAMETOOLONG, NULL, NULL},
>  	{&file_erofs,   FILE_EROFS,   EROFS,        NULL, NULL},
> +	{&file_efault,	FILE_EFAULT,  EFAULT,  setup_emem, cleanup_emem},
    {&file_efault,  "Invalid address",  EFAULT,  setup_emem, cleanup_emem},
(as Cyril suggested)

Actually this second version does only single thing (unlike the previous version
[1]), thus I suggest to merge it:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Therefore I reopen it in patchwork [2].

And you can send warning cleanup after merging this?

Could you, please, next time put version to your patchset (e.g. -v2 for second
version), this help to avoid confusions?

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20240606065506.1686-1-lufei@uniontech.com/
[2] https://patchwork.ozlabs.org/project/ltp/patch/20240624015245.54968-2-lufei@uniontech.com/

>  };

>  static void setup(void)