On 07/23/2013 10:43 AM, Tyler Hicks wrote: > One minor nitpick/question... > > On 2013-07-23 08:36:12, Steve Beattie wrote: >> Subject: [parser patch] fix apparmor cache tempfile location to use passed >> arg >> >> This patch fixes problems in the handling of both the final cache >> name location and the temporary cache file when an alternate location >> is specified. >> >> The first issue is that if the alternate cache directory location was >> specified, the alternate directory name would be used as the final location >> for >> the cache file, rather than the alternate directory + the basename of >> the profile. >> >> The second issue is that it would generate the temporary file that it >> stores the cache file in [basedir]/cache even if an alternate cache >> location was specified on the command line. This causes a problem >> if [basedir]/cache is on a separate device than the alternate cache >> location, because the rename() of the tempfile into the final location >> would fail (which the parser would not check the return code of). It >> will also break if the filesystem the basedir is located on is mounted >> read-only, which would be a motivating reason to use an alternate >> cache location. >> >> This patch fixes the above by incorporating the basename into the cache >> file name if the alternate cache location has been specified, bases the >> temporary cache file name on the destination cache name (such that they >> end up in the same directory), and finally detects if the rename fails >> and unlinks the temporary file if that happens (rather than leave it >> around). >> >> Signed-off-by: Steve Beattie <[email protected]> >> >> --- >> parser/parser_main.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> Index: b/parser/parser_main.c >> =================================================================== >> --- a/parser/parser_main.c >> +++ b/parser/parser_main.c >> @@ -1069,8 +1069,7 @@ int process_profile(int option, char *pr >> if ((profilename && option != OPTION_REMOVE) && !force_complain && >> !skip_cache) { >> if (cacheloc) { >> - cachename = strdup(cacheloc); >> - if (!cachename) { >> + if (asprintf(&cachename, "%s/%s", cacheloc, >> basename)<0) { >> PERROR(_("Memory allocation error.")); >> exit(1); >> } >> @@ -1089,7 +1088,7 @@ int process_profile(int option, char *pr >> } >> if (write_cache) { >> /* Otherwise, set up to save a cached copy */ >> - if (asprintf(&cachetemp, "%s/%s/%s-XXXXXX", basedir, >> "cache", basename)<0) { >> + if (asprintf(&cachetemp, "%s-XXXXXX", cachename)<0) { >> perror("asprintf"); >> exit(1); >> } >> @@ -1147,8 +1146,11 @@ out: >> } >> >> if (useable_cache) { >> - rename(cachetemp, cachename); >> - if (show_cache) >> + if (rename(cachetemp, cachename) < 0) { >> + PERROR("Warning failed to write cache: %s\n", >> cachename); > > I've been curious about the policy surrounding -q. Here's the option's > description: > > -q, --quiet > Do not report on the profiles as they are > loaded, and not show warnings. > > I see there's a pwarn() that respects -q and that there's one PERROR() > call conditional on -q. This message starts with "Warning", so it seems > like it should also respect -q. > hrmmm, yeah the warn vs. error is something we should really cleanup and make consistent
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
