[apparmor] [PATCH 1/2] parser: Fix cache file mtime regression

[Seth Arnold]

On Tue, Aug 11, 2015 at 09:53:53PM -0500, Tyler Hicks wrote:
> This patch fixes a regression in setting the cache file's timestamp
> handling that was introduced in r3079:
> 
>   Set cache file tstamp to the mtime of most recent policy file tstamp
> 
> The previously used utimes(2) syscall requires a two element timeval
> array. The first element in the array is the atime to be used and the
> second element is the mtime. The equivalent of a one element timeval
> array was being passed to it, resulting in garbage being used for the
> mtime value. The utimes(2) syscall either failed, due to the invalid
> input, or set mtime to an unexpected value. The return code wasn't being
> checked so the failure went unknown.
> 
> This patch switches to utimensat(2) for a couple reasons. The UTIME_OMIT
> special value allows us to preserve the inode's atime without calling
> stat(2) to fetch the atime. It also allows for nanosecond precision
> which better aligns with what stat(2) returns on the input profile and
> abstraction files. That means that we can have the exact same mtime on
> the input profile or abstraction file and the output cache file.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>

Nice catches all around.

Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks

> ---
>  parser/policy_cache.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/parser/policy_cache.c b/parser/policy_cache.c
> index 4ba732c..9dd4b11 100644
> --- a/parser/policy_cache.c
> +++ b/parser/policy_cache.c
> @@ -18,6 +18,7 @@
>  
>  #include <ctype.h>
>  #include <dirent.h>
> +#include <fcntl.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <stdlib.h>
> @@ -25,8 +26,6 @@
>  #include <sys/types.h>
>  #include <unistd.h>
>  #include <sys/stat.h>
> -#include <sys/time.h>
> -#include <utime.h>
>  
>  #include "lib.h"
>  #include "parser.h"
> @@ -166,12 +165,21 @@ void install_cache(const char *cachetmpname, const char *cachename)
>  	/* Only install the generate cache file if it parsed correctly
>  	   and did not have write/close errors */
>  	if (cachetmpname) {
> -		struct timeval t;
> +		struct timespec times[2];
> +
>  		/* set the mtime of the cache file to the most newest mtime
>  		 * of policy files used to generate it
>  		 */
> -		TIMESPEC_TO_TIMEVAL(&t, &mru_policy_tstamp);
> -		utimes(cachetmpname, &t);
> +		times[0].tv_sec = 0;
> +		times[0].tv_nsec = UTIME_OMIT;
> +		times[1] = mru_policy_tstamp;
> +		if (utimensat(AT_FDCWD, cachetmpname, times, 0) < 0) {
> +			PERROR("%s: Failed to set the mtime of cache file '%s': %m\n",
> +			       progname, cachename);
> +			unlink(cachetmpname);
> +			return;
> +		}
> +
>  		if (rename(cachetmpname, cachename) < 0) {
>  			pwarn("Warning failed to write cache: %s\n", cachename);
>  			unlink(cachetmpname);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150811/7523e886/attachment-0001.pgp>