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

[Steve Beattie]

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>
---
 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>
---
 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/
-------------- 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/20130920/60a45ace/attachment-0001.pgp>