[PATCH 4/8] busybox -- libselinux utilities applets

[Bernhard Fischer]

On Mon, Jan 29, 2007 at 11:06:25PM +0900, KaiGai Kohei wrote:
>Denis, Thanks for your comments.
>
>Denis Vlasenko wrote:
>>On Thursday 25 January 2007 15:44, KaiGai Kohei wrote:
>>>[4/8] busybox-libselinux-04-getsebool.patch
>>>  getsebool reports the a particular or all SELinux
>>>  boolean variable.
>>>  SELinux boolean variable is a interface to configure
>>>  the condition of security policy. We can enable or
>>>  disable the part of the security policy via boolean
>>>  variable.
>>>
>>>Signed-off-by: Hiroshi Shinji <shiroshi at my.email.ne.jp>
>>>Signed-off-by: KaiGai Kohei <kaigai at kaigai.gr.jp>
>>>
>>>--
>>>KaiGai Kohei <kaigai at kaigai.gr.jp>
>>
>>--- selinux/getsebool.c (revision 0)
>>+++ selinux/getsebool.c (revision 0)
>>@@ -0,0 +1,98 @@
>>+/*
>>+ * getsebool
>>+ *
>>+ * Based on libselinux 1.33.1
>>+ * Port to BusyBox  Hiroshi Shinji <shiroshi at my.email.ne.jp>
>>+ *
>>+ */
>>+
>>+#include "busybox.h"
>>+#include <unistd.h>
>>+#include <stdlib.h>
>>+#include <stdio.h>
>>+#include <getopt.h>
>>+#include <errno.h>
>>+#include <string.h>
>>+#include <selinux/selinux.h>
>
>I removed above redundant headers.
>
>>+#define GETSEBOOL_OPT_ALL      1
>>+
>>+int getsebool_main(int argc, char **argv)
>>+{
>>+       int i, rc = 0, active, pending, len = 0;
>>+       char **names;
>>+       unsigned long opt;
>>+
>>+       opt = getopt32(argc, argv, "a");
>>+
>>+       if(opt & BB_GETOPT_ERROR) {
>>+               bb_show_usage();
>>+       }
>>
>>Is it needed? I mean, can you give an example where it is needed?
>
>No. The above block is unnecessary.
>
>>+
>>+       if (!len) {
>>+               if (argc < 2)
>>+                       bb_show_usage();
>>+               len = argc - 1;
>>+               names = malloc(sizeof(char *) * len);
>>+               if (!names) {
>>+                       bb_error_msg_and_die("out of memory");
>>+               }
>>
>>xmalloc will do dying for you! :)
>>
>>+               for (i = 0; i < len; i++) {
>>+                       names[i] = strdup(argv[i + 1]);
>>
>>xstrdup. Gotta love busybox. We love to die, and love to get rid
>>of useless error paths.
>
>Thanks for the useful information.
>I replaced them with xmalloc() and xstrdup().
>
>>+      out:
>>+       for (i = 0; i < len; i++)
>>+               free(names[i]);
>>+       free(names);
>>
>>	Add if (ENABLE_FEATURE_CLEAN_UP) in front of for().
>
>OK, I appended if (ENABLE_FEATURE_CLEAN_UP) { ... } block.
>
>BTW, I found both '#if ENABLE_FEATURE_CLEAN_UP' and 'if 
>(ENABLE_FEATURE_CLEAN_UP)'
>in the source tree. Which manner is preferable?

The prefered one is in C (rather than preprocessor).
See below..
>
>Thanks,
>-- 
>KaiGai Kohei <kaigai at kaigai.gr.jp>

>Index: selinux/getsebool.c
>===================================================================
>--- selinux/getsebool.c	(revision 0)
>+++ selinux/getsebool.c	(revision 0)
>@@ -0,0 +1,83 @@
>+/*
>+ * getsebool
>+ *
>+ * Based on libselinux 1.33.1
>+ * Port to BusyBox  Hiroshi Shinji <shiroshi at my.email.ne.jp>
>+ *
>+ */
>+
>+#include "busybox.h"
>+#include <selinux/selinux.h>
>+
>+#define GETSEBOOL_OPT_ALL	1
>+
>+int getsebool_main(int argc, char **argv)
>+{
>+	int i, rc = 0, active, pending, len = 0;
>+	char **names;
>+	unsigned long opt;
>+
>+	opt = getopt32(argc, argv, "a");
>+
>+	if(opt & GETSEBOOL_OPT_ALL) {

missing space after "if"

>+		if (argc > 2)
>+			bb_show_usage();
>+		if (is_selinux_enabled() <= 0) {
>+			bb_error_msg_and_die("SELinux is disabled");

You're doing this alot. Please move this out to a
int xis_selinux_enabled(void) {
	smallint ret = is_selinux_enabled();
	if (ret != 1)
		bb_error_msg_and_die("SELinux is disabled");
	return ret;
}
in e.g. libbb/xfuncs.c and use it in your other SElinux applets, too.

>+		}
>+		errno = 0;

hm?

>+		rc = security_get_boolean_names(&names, &len);
>+		if (rc) {
->+			bb_error_msg_and_die("cannot get boolean names:  %s",
->+					     strerror(errno));

bb_perror_msg_and_die("cannot get boolean name");
should do too

>+		}
>+		if (!len) {
>+			printf("No booleans\n");

puts smaller?
>+			return 0;
>+		}
>+	}

See how you didn't use opt much?
I'd rather say 
xis_selinux_enabled();
opt_complementary="-1";/* need at least 1 non-option arg*/
if (getopt32(argc, argv, "a")) {
	rc = security_get_boolean_names(&names, &len);
	if (rc ...
}

>+
>+	if (is_selinux_enabled() <= 0)
>+		bb_error_msg_and_die("SELinux is disabled");

That can't be right, no?
You called security_get_boolean_names() before checking if selinux is
enabled or not. Does this work?

What about removing that is_selinux_enabled block here, move the call to 
xis_selinux_enabled from the "if(opt & GETSEBOOL_OPT_ALL) {" block to
below the "if (opt..)" block so you check for enabled only once (before
get_boolean_nam())

>+
>+	if (!len) {
>+		if (argc < 2)
>+			bb_show_usage();
>+		len = argc - 1;
>+		names = xmalloc(sizeof(char *) * len);
>+		for (i = 0; i < len; i++)
>+			names[i] = xstrdup(argv[i + 1]);
>+	}
>+
>+	for (i = 0; i < len; i++) {
>+		active = security_get_boolean_active(names[i]);
>+		if (active < 0) {
>+			bb_error_msg("error getting active value for %s", names[i]);
>+			rc = -1;
>+			goto out;
>+		}
>+		pending = security_get_boolean_pending(names[i]);
>+		if (pending < 0) {
>+			bb_error_msg("error getting pending value for %s", names[i]);
>+			rc = -1;
>+			goto out;
>+		}
>+		if (pending != active) {
>+			printf("%s --> %s pending: %s\n", names[i],
>+			       (active ? "on" : "off"),
>+			       (pending ? "on" : "off"));
>+		} else {
>+			printf("%s --> %s\n", names[i],
>+			       (active ? "on" : "off"));
>+		}
>+	}
>+
>+      out:
>+	if (ENABLE_FEATURE_CLEAN_UP) {
>+		for (i = 0; i < len; i++)
>+			free(names[i]);
>+		free(names);
>+	}
>+
>+	return rc;
>+}

>_______________________________________________
>busybox mailing list
>busybox at busybox.net
>http://busybox.net/cgi-bin/mailman/listinfo/busybox