[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apparmor-dev
Subject:    Re: [apparmor] [patch v2] tests: readdir - test both getdents() and getdents64() if available
From:       Tyler Hicks <tyhicks () canonical ! com>
Date:       2017-04-05 21:09:15
Message-ID: 5a57ae89-8163-1fbf-3a68-56672dfe78e8 () canonical ! com
[Download RAW message or body]

[Attachment #2 (multipart/signed)]

[Attachment #4 (multipart/mixed)]


On 04/05/2017 01:57 PM, Steve Beattie wrote:
> On Tue, Apr 04, 2017 at 03:41:41PM -0500, Tyler Hicks wrote:
>> I didn't mean to make this simple test improvement turn into something
>> complex. I'm willing to ack your original patch if you don't see a quick
>> and easy solution to this issue.
> 
> Nah, let's do it right. V2 of the patch follows. Changes since v1:
> 
>  - compile error if neither SYS_getdents or SYS_getdents is defined
>  - verify that getdents() and getdents64() return the same value if both
>    are available.
>  - propagate errno from getdents/getdents64 if failure occurs, instead
>    of synthetic error value.
> 
> (For reviewing, because so much changes, it might just be easier to
> review readdir.c after applying, rather than the diff itself.)
> 
> Subject: tests: readdir - test both getdents() and getdents64() if available
> 
> In commit 3649, Colin King fixed the readdir test build issue where
> aarch64 only supports getdetns64(), not getdents(). Realistically,
> however, we want to ensure mediation occurs on both syscalls where
> they exist. This patch changes the test to attempt performing both
> versions of getdents(). Also, if both versions exist, return a
> failure if the result of each differs from the other.
> 
> Bug: https://bugs.launchpad.net/bugs/1674245
> 
> Signed-off-by: Steve Beattie <steve@nxnw.org>
> ---
>  tests/regression/apparmor/readdir.c |  101 +++++++++++++++++++++++++++---------
>  1 file changed, 76 insertions(+), 25 deletions(-)
> 
> Index: b/tests/regression/apparmor/readdir.c
> ===================================================================
> --- a/tests/regression/apparmor/readdir.c
> +++ b/tests/regression/apparmor/readdir.c
> @@ -1,10 +1,14 @@
>  /*
>   *	Copyright (C) 2002-2006 Novell/SUSE
> + *	Copyright (C) 2017 Canonical, Ltd.
>   *
>   *	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, version 2 of the
>   *	License.
> + *
> + *	We attempt to test both getdents() and getdents64() here, but
> + *	some architectures like aarch64 only support getdents64().
>   */
>  #define _GNU_SOURCE
>  
> @@ -18,45 +22,92 @@
>  #include <string.h>
>  #include <sys/syscall.h>
>  
> -int main(int argc, char *argv[])
> +/* error if neither SYS_getdents or SYS_getdents64 is defined */
> +#if !defined(SYS_getdents) && !defined(SYS_getdents64)
> +  #error "Neither SYS_getdents or SYS_getdents64 is defined, something has gone wrong!"
> +#endif
> +
> +#ifdef SYS_getdents
> +int my_readdir(char *dirname)
>  {
>  	int fd;
> -#if defined(SYS_getdents64)
> -	struct dirent64 dir;
> -#else
>  	struct dirent dir;
> -#endif
>  
> -	if (argc != 2){
> -		fprintf(stderr, "usage: %s dir\n",
> -			argv[0]);
> -		return 1;
> +	fd = open(dirname, O_RDONLY, 0);
> +	if (fd == -1) {
> +		printf("FAIL - open  %s\n", strerror(errno));
> +		return errno;
>  	}
>  
> -	fd = open(argv[1], O_RDONLY, 0);
> -	if (fd == -1){
> -		printf("FAIL - open  %s\n", strerror(errno));
> -		return 1;
> +	/* getdents isn't exported by glibc, so must use syscall() */
> +	if (syscall(SYS_getdents, fd, &dir, sizeof(dir)) == -1){
> +		printf("FAIL - getdents  %s\n", strerror(errno));
> +		close(fd);
> +		return errno;
>  	}
>  
> -	/*
> -	if (fchdir(fd) == -1){
> -		printf("FAIL - fchdir  %s\n", strerror(errno));
> -		return 1;
> +	close(fd);
> +	return 0;
> +}
> +#endif
> +
> +#ifdef SYS_getdents64
> +int my_readdir64(char *dirname)
> +{
> +	int fd;
> +	struct dirent64 dir;
> +
> +	fd = open(dirname, O_RDONLY, 0);
> +	if (fd == -1) {
> +		printf("FAIL - open  %s\n", strerror(errno));
> +		return errno;
>  	}
> -	*/
>  
>  	/* getdents isn't exported by glibc, so must use syscall() */
> -#if defined(SYS_getdents64)
> -	if (syscall(SYS_getdents64, fd, &dir, sizeof(struct dirent64)) == -1){
> -#else
> -	if (syscall(SYS_getdents, fd, &dir, sizeof(struct dirent)) == -1){
> +	if (syscall(SYS_getdents64, fd, &dir, sizeof(dir)) == -1){
> +		printf("FAIL - getdents64  %s\n", strerror(errno));
> +		close(fd);
> +		return errno;
> +	}
> +
> +	close(fd);
> +	return 0;
> +}
>  #endif
> -		printf("FAIL - getdents  %s\n", strerror(errno));
> -		return 1;
> +
> +int main(int argc, char *argv[])
> +{
> +	int rc = 0, rc64 = 0, err = 0;
> +
> +	if (argc != 2) {
> +		fprintf(stderr, "usage: %s dir\n",
> +			argv[0]);
> +		err = 1;
> +		goto err;
> +	}
> +
> +#ifdef SYS_getdents
> +	rc = my_readdir(argv[1]);
> +	err |= rc;
> +#endif
> +
> +#ifdef SYS_getdents64
> +	rc64 = my_readdir64(argv[1]);
> +	err |= rc64;
> +#endif
> +
> +#if defined(SYS_getdents) && defined(SYS_getdents64)
> +	if (rc != rc64) {
> +		printf("FAIL - getdents and getdents64 returned different values: %d vs %d\n", rc, rc64);

I feel like this should be ERROR instead of FAIL. If we use FAIL here,
the test will "pass" in an expected fail test case when getdents()
returns something different than getdents64(). I don't see a way around
that other than printing a different (unexpected) output than FAIL.

swap.sh looks to be the only other test using ERROR. prologue.inc
doesn't know about ERROR and handles it by calling testerror().

We're getting into extreme corner case territory here. As before, I'm
prepared to declare "good enough". Let me know your thoughts.

Tyler

> +		err = 1;
>  	}
> +#endif
> +
> +	if (err != 0)
> +		goto err;
>  
>  	printf("PASS\n");
>  
> -	return 0;
> +err:
> +	return err;
>  }
> 
> 
> 



["signature.asc" (application/pgp-signature)]

-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor


[prev in list] [next in list] [prev in thread] [next in thread]