[apparmor] [PATCH 8/8] Convert codomain to a class

[Tyler Hicks]

On 2013-09-20 17:16:01, Steve Beattie wrote:
> On Fri, Sep 20, 2013 at 12:29:31PM -0700, Steve Beattie wrote:
> > On Fri, Sep 20, 2013 at 12:26:11PM -0700, Steve Beattie wrote:
> > > On Wed, Sep 11, 2013 at 01:47:47AM -0700, Tyler Hicks wrote:
> > > > From: John Johansen <john.johansen at canonical.com>
> > > > 
> > > > Convert the codomain to a class, and the policy lists that store
> > > > codomains to stl containers instead of glibc twalk.
> > > > 
> > > > Signed-off-by: John Johansen <john.johansen at canonical.com>
> > > > [tyhicks: Merge with dbus changes and process_file_entries() cleanup]
> > > > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > > 
> > > There's still a problem with this patch (even with the other fixes I
> > > made to it), it's not loading profiles with multiple hats properly:
> > > 
> > >   $ cat /tmp/example_profile
> > >   /tests/regression/apparmor/changehat {
> > > 
> > >     ^sub {
> > >       /proc/*/attr/current w,
> > >       /tmp/sdtest.1713-15650-z0Mlub/file2 rw,
> > >     }
> > > 
> > >     ^sub2 {
> > >       /proc/*/attr/current w,
> > >       /tmp/sdtest.1713-15650-z0Mlub/file2 rw,
> > >     }
> > > 
> > >     ^sub3 {
> > >       /proc/*/attr/current w,
> > >       /tmp/sdtest.1713-15650-z0Mlub/file2 rw,
> > >     }
> > >   }
> > > 
> > >   $ sudo ./apparmor_parser /tmp/example_profile
> > > 
> > >   $ sudo grep changehat /sys/kernel/security/apparmor/profiles
> > >   /tests/regression/apparmor/changehat (enforce)
> > >   /tests/regression/apparmor/changehat//sub (enforce)
> 
> Alright, I traced it down and the following patch fixes it:
> 
> Signed-off-by: Steve Beattie <steve at nxnw.org>

This looks good to me.

> ---
>  parser/parser_interface.c |    2 +-
>  parser/parser_policy.c    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Index: b/parser/parser_interface.c
> ===================================================================
> --- a/parser/parser_interface.c
> +++ b/parser/parser_interface.c
> @@ -837,7 +837,7 @@ int __sd_serialize_profile(int option, P
>  	close(fd);
>  
>  	if (!prof->hat_table.empty() && option != OPTION_REMOVE) {
> -		if (load_flattened_hats(prof, option) != 0)
> +		if (load_flattened_hats(prof, option) == 0)
>  			return 0;
>  	}
>  
> Index: b/parser/parser_policy.c
> ===================================================================
> --- a/parser/parser_policy.c
> +++ b/parser/parser_policy.c
> @@ -248,7 +248,7 @@ int load_policy_list(ProfileList &list,
>  
>  	for (ProfileList::iterator i = list.begin(); i != list.end(); i++) {
>  		res = load_profile(option, *i);
> -		if (!res)
> +		if (res != 0)
>  			break;
>  	}
>  
> 
> The problem is a result of our sometimes returning non-zero to indicate
> errors and sometimes zero for errors.
> 
> Tracking it down was made slightly more confusing because the original
> patch ended up creating two functions named sd_serialize_profile,
> with different parameters. While that's legal in C++ (name binding
> includes the argument types), it was confusing for this human to
> understand. Thus, I'd like to see something like the following as well
> (alternate suggestions for renaming accepted):
> 
> Signed-off-by: Steve Beattie <steve at nxnw.org>
> ---

I agree that it is a good idea to have different function names. I'm not
familiar enough with the code to suggest a better name right now, so
lets go with the __ prefix.

I'll roll both of these patches in.

Tyler

>  parser/parser_interface.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: b/parser/parser_interface.c
> ===================================================================
> --- a/parser/parser_interface.c
> +++ b/parser/parser_interface.c
> @@ -60,7 +60,7 @@
>  
>  #define SUBDOMAIN_INTERFACE_DFA_VERSION 5
>  
> -int sd_serialize_profile(int option, Profile *prof);
> +int __sd_serialize_profile(int option, Profile *prof);
>  
>  static void print_error(int error)
>  {
> @@ -107,7 +107,7 @@ int load_profile(int option, Profile *pr
>  	int error = 0;
>  
>  	PDEBUG("Serializing policy for %s.\n", prof->name);
> -	retval = sd_serialize_profile(option, prof);
> +	retval = __sd_serialize_profile(option, prof);
>  
>  	if (retval < 0) {
>  		error = retval;	/* yeah, we'll just report the last error */
> @@ -700,7 +700,7 @@ int sd_serialize_top_profile(sd_serializ
>  }
>  
>  int cache_fd = -1;
> -int sd_serialize_profile(int option, Profile *prof)
> +int __sd_serialize_profile(int option, Profile *prof)
>  {
>  	int fd = -1;
>  	int error = -ENOMEM, size, wsize;
> 
> -- 
> Steve Beattie
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/



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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130927/c90b26eb/attachment.pgp>