All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/4] semodule_expand: update
@ 2023-07-06 14:53 Christian Göttsche
  2023-07-06 14:53 ` [PATCH v2 2/4] semodule_link: update Christian Göttsche
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Christian Göttsche @ 2023-07-06 14:53 UTC (permalink / raw)
  To: selinux

Drop unnecessary declarations.
Reduce scope of file global variable.
Mention -v argument in help usage message.
More strict integer conversion.
More strict argument count checking.
Check closing file for incomplete write.
Rework resource cleanup, so that all files and allocated memory are
released in all branches, useful to minimize reports while debugging
libsepol under valgrind(8) or sanitizers.
Add help argument option -h.
Set close-on-exec flag in case of any sibling threads.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - address comments by Jim:
    * drop exit() calls
    * reduce to only one final return statement
  - add help argument option -h
  - set close-on-exec flag
---
 .../semodule_expand/semodule_expand.8         |   5 +-
 .../semodule_expand/semodule_expand.c         | 112 +++++++++++-------
 2 files changed, 73 insertions(+), 44 deletions(-)

diff --git a/semodule-utils/semodule_expand/semodule_expand.8 b/semodule-utils/semodule_expand/semodule_expand.8
index 1b482a1f..84b943cd 100644
--- a/semodule-utils/semodule_expand/semodule_expand.8
+++ b/semodule-utils/semodule_expand/semodule_expand.8
@@ -3,7 +3,7 @@
 semodule_expand \- Expand a SELinux policy module package.
 
 .SH SYNOPSIS
-.B semodule_expand [-V ] [ -a ] [ -c [version]] basemodpkg outputfile
+.B semodule_expand [-ahV] [ -c [version]] basemodpkg outputfile
 .br
 .SH DESCRIPTION
 .PP
@@ -17,6 +17,9 @@ together a set of packages into a single package).
 
 .SH "OPTIONS"
 .TP
+.B \-h
+show help
+.TP
 .B \-V
 show version
 .TP
diff --git a/semodule-utils/semodule_expand/semodule_expand.c b/semodule-utils/semodule_expand/semodule_expand.c
index 895cdd78..99380abe 100644
--- a/semodule-utils/semodule_expand/semodule_expand.c
+++ b/semodule-utils/semodule_expand/semodule_expand.c
@@ -21,32 +21,25 @@
 #include <unistd.h>
 #include <string.h>
 
-extern char *optarg;
-extern int optind;
-
-int policyvers = 0;
-
 #define EXPANDPOLICY_VERSION "1.0"
 
-static __attribute__((__noreturn__)) void usage(const char *program_name)
+static void usage(const char *program_name)
 {
-	printf("usage: %s [-V -a -c [version]] basemodpkg outputfile\n",
+	printf("usage: %s [-h -V -a -c [version] -v] basemodpkg outputfile\n",
 	       program_name);
-	exit(1);
 }
 
 int main(int argc, char **argv)
 {
-	char *basename, *outname;
-	int ch, ret, show_version = 0, verbose = 0;
-	struct sepol_policy_file *pf;
-	sepol_module_package_t *base;
-	sepol_policydb_t *out, *p;
-	FILE *fp, *outfile;
-	int check_assertions = 1;
-	sepol_handle_t *handle;
-
-	while ((ch = getopt(argc, argv, "c:Vva")) != EOF) {
+	const char *basename, *outname;
+	int ch, ret, show_version = 0, verbose = 0, policyvers = 0, check_assertions = 1;
+	struct sepol_policy_file *pf = NULL;
+	sepol_module_package_t *base = NULL;
+	sepol_policydb_t *out = NULL, *p;
+	FILE *fp = NULL, *outfile = NULL;
+	sepol_handle_t *handle = NULL;
+
+	while ((ch = getopt(argc, argv, "c:Vvah")) != EOF) {
 		switch (ch) {
 		case 'V':
 			show_version = 1;
@@ -54,14 +47,20 @@ int main(int argc, char **argv)
 		case 'v':
 			verbose = 1;
 			break;
+		case 'h':
+			usage(argv[0]);
+			return EXIT_SUCCESS;
 		case 'c':{
-				long int n = strtol(optarg, NULL, 10);
+				long int n;
+
+				errno = 0;
+				n = strtol(optarg, NULL, 10);
 				if (errno) {
 					fprintf(stderr,
 						"%s:  Invalid policyvers specified: %s\n",
 						argv[0], optarg);
 					usage(argv[0]);
-					exit(1);
+					return EXIT_FAILURE;
 				}
 				if (n < sepol_policy_kern_vers_min()
 				    || n > sepol_policy_kern_vers_max()) {
@@ -71,7 +70,7 @@ int main(int argc, char **argv)
 						sepol_policy_kern_vers_min(),
 						sepol_policy_kern_vers_max());
 					usage(argv[0]);
-					exit(1);
+					return EXIT_FAILURE;
 				}
 				policyvers = n;
 				break;
@@ -82,6 +81,7 @@ int main(int argc, char **argv)
 			}
 		default:
 			usage(argv[0]);
+			return EXIT_FAILURE;
 		}
 	}
 
@@ -92,84 +92,90 @@ int main(int argc, char **argv)
 
 	if (show_version) {
 		printf("%s\n", EXPANDPOLICY_VERSION);
-		exit(0);
+		return EXIT_SUCCESS;
 	}
 
 	/* check args */
-	if (argc < 3 || !(optind != (argc - 1))) {
+	if (argc < 3 || argc - optind != 2) {
 		fprintf(stderr,
 			"%s:  You must provide the base module package and output filename\n",
 			argv[0]);
 		usage(argv[0]);
+		return EXIT_FAILURE;
 	}
 
 	basename = argv[optind++];
 	outname = argv[optind];
 
 	handle = sepol_handle_create();
-	if (!handle)
-		exit(1);
+	if (!handle) {
+		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+		goto failure;
+	}
 
 	if (sepol_policy_file_create(&pf)) {
 		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
 
 	/* read the base module */
 	if (sepol_module_package_create(&base)) {
 		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
-	fp = fopen(basename, "r");
+
+	fp = fopen(basename, "re");
 	if (!fp) {
 		fprintf(stderr, "%s:  Can't open '%s':  %s\n",
 			argv[0], basename, strerror(errno));
-		exit(1);
+		goto failure;
 	}
+
 	sepol_policy_file_set_fp(pf, fp);
 	ret = sepol_module_package_read(base, pf, 0);
 	if (ret) {
 		fprintf(stderr, "%s:  Error in reading package from %s\n",
 			argv[0], basename);
-		exit(1);
+		goto failure;
 	}
+
 	fclose(fp);
+	fp = NULL;
 
 	/* linking the base takes care of enabling optional avrules */
 	p = sepol_module_package_get_policy(base);
 	if (sepol_link_modules(handle, p, NULL, 0, 0)) {
 		fprintf(stderr, "%s:  Error while enabling avrules\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
 
 	/* create the output policy */
 
 	if (sepol_policydb_create(&out)) {
 		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
 
 	sepol_set_expand_consume_base(handle, 1);
 
 	if (sepol_expand_module(handle, p, out, verbose, check_assertions)) {
 		fprintf(stderr, "%s:  Error while expanding policy\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
 
 	if (policyvers) {
 		if (sepol_policydb_set_vers(out, policyvers)) {
 			fprintf(stderr, "%s:  Invalid version %d\n", argv[0],
 				policyvers);
-			exit(1);
+			goto failure;
 		}
 	}
 
-	sepol_module_package_free(base);
-
-	outfile = fopen(outname, "w");
+	outfile = fopen(outname, "we");
 	if (!outfile) {
-		perror(outname);
-		exit(1);
+		fprintf(stderr, "%s:  Can't open '%s':  %s\n",
+			argv[0], outname, strerror(errno));
+		goto failure;
 	}
 
 	sepol_policy_file_set_fp(pf, outfile);
@@ -178,12 +184,32 @@ int main(int argc, char **argv)
 		fprintf(stderr,
 			"%s:  Error while writing expanded policy to %s\n",
 			argv[0], outname);
-		exit(1);
+		goto failure;
 	}
-	fclose(outfile);
-	sepol_handle_destroy(handle);
+
+	ret = fclose(outfile);
+	outfile = NULL;
+	if (ret) {
+		fprintf(stderr, "%s:  Error closing policy file %s:  %s\n",
+			argv[0], outname, strerror(errno));
+		goto failure;
+	}
+
+	ret = EXIT_SUCCESS;
+	goto cleanup;
+
+failure:
+	ret = EXIT_FAILURE;
+
+cleanup:
+	if (outfile)
+		fclose(outfile);
 	sepol_policydb_free(out);
+	if (fp)
+		fclose(fp);
+	sepol_module_package_free(base);
 	sepol_policy_file_free(pf);
+	sepol_handle_destroy(handle);
 
-	return 0;
+	return ret;
 }
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/4] semodule_link: update
  2023-07-06 14:53 [PATCH v2 1/4] semodule_expand: update Christian Göttsche
@ 2023-07-06 14:53 ` Christian Göttsche
  2023-07-06 14:53 ` [PATCH v2 3/4] semodule_package: update Christian Göttsche
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Christian Göttsche @ 2023-07-06 14:53 UTC (permalink / raw)
  To: selinux

Drop unnecessary declarations.
More verbose error messages and add missing trailing newline.
More strict argument count checking.
Check closing file for incomplete write.
Rework resource cleanup, so that all files and allocated memory are
released in all branches, useful to minimize reports while debugging
libsepol under valgrind(8) or sanitizers.
Add help argument option -h.
Set close-on-exec flag in case of any sibling thread.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - address comment from Jim
    * avoid exit() calls
    * reduce to one final return statement
    * drop global variable progname
  - add help argument option
  - set close-on-exec flag
---
 semodule-utils/semodule_link/semodule_link.8 |  5 +-
 semodule-utils/semodule_link/semodule_link.c | 87 +++++++++++---------
 2 files changed, 53 insertions(+), 39 deletions(-)

diff --git a/semodule-utils/semodule_link/semodule_link.8 b/semodule-utils/semodule_link/semodule_link.8
index a2bda3f9..dba3d3b6 100644
--- a/semodule-utils/semodule_link/semodule_link.8
+++ b/semodule-utils/semodule_link/semodule_link.8
@@ -3,7 +3,7 @@
 semodule_link \- Link SELinux policy module packages together
 
 .SH SYNOPSIS
-.B semodule_link [-Vv] [-o outfile] basemodpkg modpkg1 [modpkg2]...
+.B semodule_link [-hVv] [-o outfile] basemodpkg modpkg1 [modpkg2]...
 .br
 .SH DESCRIPTION
 .PP
@@ -16,6 +16,9 @@ semodule_package.
 
 .SH "OPTIONS"
 .TP
+.B \-h
+show help
+.TP
 .B \-V
 show version
 .TP
diff --git a/semodule-utils/semodule_link/semodule_link.c b/semodule-utils/semodule_link/semodule_link.c
index 38a6843c..0f157bd9 100644
--- a/semodule-utils/semodule_link/semodule_link.c
+++ b/semodule-utils/semodule_link/semodule_link.c
@@ -21,18 +21,13 @@
 
 #define LINKPOLICY_VERSION "1.0"
 
-char *progname;
-extern char *optarg;
-extern int optind;
-
-static __attribute__((__noreturn__)) void usage(const char *program_name)
+static void usage(const char *program_name)
 {
-	printf("usage: %s [-Vv] [-o outfile] basemodpkg modpkg1 [modpkg2]...\n",
+	printf("usage: %s [-hVv] [-o outfile] basemodpkg modpkg1 [modpkg2]...\n",
 	       program_name);
-	exit(1);
 }
 
-static sepol_module_package_t *load_module(char *filename)
+static sepol_module_package_t *load_module(const char *filename, const char *progname)
 {
 	int ret;
 	FILE *fp = NULL;
@@ -47,9 +42,9 @@ static sepol_module_package_t *load_module(char *filename)
 		fprintf(stderr, "%s:  Out of memory\n", progname);
 		goto bad;
 	}
-	fp = fopen(filename, "r");
+	fp = fopen(filename, "re");
 	if (!fp) {
-		fprintf(stderr, "%s:  Could not open package %s:  %s", progname,
+		fprintf(stderr, "%s:  Could not open package %s:  %s\n", progname,
 			filename, strerror(errno));
 		goto bad;
 	}
@@ -76,16 +71,16 @@ static sepol_module_package_t *load_module(char *filename)
 
 int main(int argc, char **argv)
 {
-	int ch, i, show_version = 0, verbose = 0, num_mods;
-	char *basename, *outname = NULL;
-	sepol_module_package_t *base, **mods;
-	FILE *outfile;
-	struct sepol_policy_file *pf;
-
-	progname = argv[0];
+	int ch, i, ret, show_version = 0, verbose = 0, num_mods = 0;
+	const char *basename, *outname = NULL;
+	sepol_module_package_t *base = NULL, **mods = NULL;
+	struct sepol_policy_file *pf = NULL;
 
-	while ((ch = getopt(argc, argv, "o:Vv")) != EOF) {
+	while ((ch = getopt(argc, argv, "ho:Vv")) != EOF) {
 		switch (ch) {
+		case 'h':
+			usage(argv[0]);
+			return EXIT_SUCCESS;
 		case 'V':
 			show_version = 1;
 			break;
@@ -97,80 +92,96 @@ int main(int argc, char **argv)
 			break;
 		default:
 			usage(argv[0]);
+			return EXIT_FAILURE;
 		}
 	}
 
 	if (show_version) {
 		printf("%s\n", LINKPOLICY_VERSION);
-		exit(0);
+		return EXIT_SUCCESS;
 	}
 
 	/* check args */
-	if (argc < 3 || !(optind != (argc - 1))) {
+	if (argc < 3 || optind + 2 > argc) {
 		fprintf(stderr,
 			"%s:  You must provide the base module package and at least one other module package\n",
 			argv[0]);
 		usage(argv[0]);
+		return EXIT_FAILURE;
 	}
 
 	basename = argv[optind++];
-	base = load_module(basename);
+	base = load_module(basename, argv[0]);
 	if (!base) {
 		fprintf(stderr,
 			"%s:  Could not load base module from file %s\n",
 			argv[0], basename);
-		exit(1);
+		goto failure;
 	}
 
 	num_mods = argc - optind;
-	mods =
-	    (sepol_module_package_t **) malloc(sizeof(sepol_module_package_t *)
-					       * num_mods);
+	mods = calloc(num_mods, sizeof(sepol_module_package_t *));
 	if (!mods) {
 		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
-	memset(mods, 0, sizeof(sepol_module_package_t *) * num_mods);
 
 	for (i = 0; optind < argc; optind++, i++) {
-		mods[i] = load_module(argv[optind]);
+		mods[i] = load_module(argv[optind], argv[0]);
 		if (!mods[i]) {
 			fprintf(stderr,
 				"%s:  Could not load module from file %s\n",
 				argv[0], argv[optind]);
-			exit(1);
+			goto failure;
 		}
 	}
 
 	if (sepol_link_packages(NULL, base, mods, num_mods, verbose)) {
 		fprintf(stderr, "%s:  Error while linking packages\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
 
 	if (outname) {
-		outfile = fopen(outname, "w");
+		FILE *outfile = fopen(outname, "we");
 		if (!outfile) {
-			perror(outname);
-			exit(1);
+			fprintf(stderr, "%s:  Could not open output file %s:  %s\n",
+				argv[0], outname, strerror(errno));
+			goto failure;
 		}
 
 		if (sepol_policy_file_create(&pf)) {
 			fprintf(stderr, "%s:  Out of memory\n", argv[0]);
-			exit(1);
+			fclose(outfile);
+			goto failure;
 		}
 		sepol_policy_file_set_fp(pf, outfile);
 		if (sepol_module_package_write(base, pf)) {
 			fprintf(stderr, "%s:  Error writing linked package.\n",
 				argv[0]);
-			exit(1);
+			sepol_policy_file_free(pf);
+			fclose(outfile);
+			goto failure;
 		}
 		sepol_policy_file_free(pf);
-		fclose(outfile);
+
+		if (fclose(outfile)) {
+			fprintf(stderr, "%s:  Error closing linked package:  %s\n",
+				argv[0], strerror(errno));
+			goto failure;
+		}
 	}
 
-	sepol_module_package_free(base);
+	ret = EXIT_SUCCESS;
+	goto cleanup;
+
+failure:
+	ret = EXIT_FAILURE;
+
+cleanup:
 	for (i = 0; i < num_mods; i++)
 		sepol_module_package_free(mods[i]);
 	free(mods);
-	exit(0);
+	sepol_module_package_free(base);
+
+	return ret;
 }
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/4] semodule_package: update
  2023-07-06 14:53 [PATCH v2 1/4] semodule_expand: update Christian Göttsche
  2023-07-06 14:53 ` [PATCH v2 2/4] semodule_link: update Christian Göttsche
@ 2023-07-06 14:53 ` Christian Göttsche
  2023-07-06 14:53 ` [PATCH v2 4/4] semodule_unpackage: update Christian Göttsche
  2023-07-19 15:58 ` [PATCH v2 1/4] semodule_expand: update James Carter
  3 siblings, 0 replies; 6+ messages in thread
From: Christian Göttsche @ 2023-07-06 14:53 UTC (permalink / raw)
  To: selinux

Drop unnecessary declarations.
Add missing error messages.
More strict command line argument parsing.
Check closing file for incomplete write.
Rework resource cleanup, so that all files and allocated memory are
released in all branches, useful to minimize reports while debugging
libsepol under valgrind(8) or sanitizers.
Set close-on-exec flag in case of any sibling thread.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - address comment from Jim
    * drop exit() calls
    * reduce to one final return statement
    * drop unnecessary ? option handling
    * drop global variable progname
  - set close-on-exec flag
---
 .../semodule_package/semodule_package.8       |   3 +
 .../semodule_package/semodule_package.c       | 227 +++++++++++-------
 2 files changed, 139 insertions(+), 91 deletions(-)

diff --git a/semodule-utils/semodule_package/semodule_package.8 b/semodule-utils/semodule_package/semodule_package.8
index 9697cc55..725d3d15 100644
--- a/semodule-utils/semodule_package/semodule_package.8
+++ b/semodule-utils/semodule_package/semodule_package.8
@@ -42,6 +42,9 @@ File contexts file for the module (optional).
 .TP
 .B  \-n \-\-nc <netfilter context file>
 netfilter context file to be included in the package.
+.TP
+.B  \-h \-\-help
+Show help message.
 
 .SH SEE ALSO
 .B checkmodule(8), semodule(8), semodule_unpackage(8)
diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c
index bc8584b5..4cb6f5a3 100644
--- a/semodule-utils/semodule_package/semodule_package.c
+++ b/semodule-utils/semodule_package/semodule_package.c
@@ -19,10 +19,7 @@
 #include <fcntl.h>
 #include <errno.h>
 
-char *progname = NULL;
-extern char *optarg;
-
-static __attribute__((__noreturn__)) void usage(const char *prog)
+static void usage(const char *prog)
 {
 	printf("usage: %s -o <output file> -m <module> [-f <file contexts>]\n",
 	       prog);
@@ -34,34 +31,14 @@ static __attribute__((__noreturn__)) void usage(const char *prog)
 	printf
 	    ("  -u --user_extra	user_extra file (only valid in base)\n");
 	printf("  -n --nc		Netfilter contexts file\n");
-	exit(1);
+	printf("  -h --help		Show this help message\n");
 }
 
-static int file_to_policy_file(const char *filename, struct sepol_policy_file **pf,
-			       const char *mode)
-{
-	FILE *f;
-
-	if (sepol_policy_file_create(pf)) {
-		fprintf(stderr, "%s:  Out of memory\n", progname);
-		return -1;
-	}
-
-	f = fopen(filename, mode);
-	if (!f) {
-		fprintf(stderr, "%s:  Could not open file %s:  %s\n", progname,
-			strerror(errno), filename);
-		return -1;
-	}
-	sepol_policy_file_set_fp(*pf, f);
-	return 0;
-}
-
-static int file_to_data(const char *path, char **data, size_t * len)
+static int file_to_data(const char *path, char **data, size_t * len, const char *progname)
 {
 	int fd;
 	struct stat sb;
-	fd = open(path, O_RDONLY);
+	fd = open(path, O_RDONLY | O_CLOEXEC);
 	if (fd < 0) {
 		fprintf(stderr, "%s:  Failed to open %s:  %s\n", progname, path,
 			strerror(errno));
@@ -94,17 +71,18 @@ static int file_to_data(const char *path, char **data, size_t * len)
 
 int main(int argc, char **argv)
 {
-	struct sepol_module_package *pkg;
-	struct sepol_policy_file *mod, *out;
+	struct sepol_module_package *pkg = NULL;
+	struct sepol_policy_file *mod = NULL, *out = NULL;
+	FILE *fp = NULL;
 	char *module = NULL, *file_contexts = NULL, *seusers =
 	    NULL, *user_extra = NULL;
 	char *fcdata = NULL, *outfile = NULL, *seusersdata =
 	    NULL, *user_extradata = NULL;
 	char *netfilter_contexts = NULL, *ncdata = NULL;
 	size_t fclen = 0, seuserslen = 0, user_extralen = 0, nclen = 0;
-	int i;
+	int i, ret;
 
-	static struct option opts[] = {
+	const struct option opts[] = {
 		{"module", required_argument, NULL, 'm'},
 		{"fc", required_argument, NULL, 'f'},
 		{"seuser", required_argument, NULL, 's'},
@@ -119,146 +97,213 @@ int main(int argc, char **argv)
 		switch (i) {
 		case 'h':
 			usage(argv[0]);
-			exit(0);
+			return EXIT_SUCCESS;
 		case 'm':
 			if (module) {
 				fprintf(stderr,
 					"May not specify more than one module\n");
-				exit(1);
+				return EXIT_FAILURE;
 			}
 			module = strdup(optarg);
-			if (!module)
-				exit(1);
+			if (!module) {
+				fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+				return EXIT_FAILURE;
+			}
 			break;
 		case 'f':
 			if (file_contexts) {
 				fprintf(stderr,
 					"May not specify more than one file context file\n");
-				exit(1);
+				return EXIT_FAILURE;
 			}
 			file_contexts = strdup(optarg);
-			if (!file_contexts)
-				exit(1);
+			if (!file_contexts) {
+				fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+				return EXIT_FAILURE;
+			}
 			break;
 		case 'o':
 			if (outfile) {
 				fprintf(stderr,
 					"May not specify more than one output file\n");
-				exit(1);
+				return EXIT_FAILURE;
 			}
 			outfile = strdup(optarg);
-			if (!outfile)
-				exit(1);
+			if (!outfile) {
+				fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+				return EXIT_FAILURE;
+			}
 			break;
 		case 's':
 			if (seusers) {
 				fprintf(stderr,
 					"May not specify more than one seuser file\n");
-				exit(1);
+				return EXIT_FAILURE;
 			}
 			seusers = strdup(optarg);
-			if (!seusers)
-				exit(1);
+			if (!seusers) {
+				fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+				return EXIT_FAILURE;
+			}
 			break;
 		case 'u':
 			if (user_extra) {
 				fprintf(stderr,
 					"May not specify more than one user_extra file\n");
-				exit(1);
+				return EXIT_FAILURE;
 			}
 			user_extra = strdup(optarg);
-			if (!user_extra)
-				exit(1);
+			if (!user_extra) {
+				fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+				return EXIT_FAILURE;
+			}
 			break;
 		case 'n':
 			if (netfilter_contexts) {
 				fprintf(stderr,
 					"May not specify more than one netfilter contexts file\n");
-				exit(1);
+				return EXIT_FAILURE;
 			}
 			netfilter_contexts = strdup(optarg);
-			if (!netfilter_contexts)
-				exit(1);
+			if (!netfilter_contexts) {
+				fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+				return EXIT_FAILURE;
+			}
 			break;
+		default:
+			usage(argv[0]);
+			return EXIT_FAILURE;
 		}
 	}
 
-	progname = argv[0];
+	if (optind < argc) {
+		fprintf(stderr, "%s:  Superfluous command line arguments: ", argv[0]);
+		while (optind < argc)
+			 fprintf(stderr, "%s ", argv[optind++]);
+		fprintf(stderr, "\n");
+		usage(argv[0]);
+		return EXIT_FAILURE;
+	}
 
 	if (!module || !outfile) {
 		usage(argv[0]);
-		exit(0);
+		return EXIT_FAILURE;
 	}
 
-	if (file_contexts) {
-		if (file_to_data(file_contexts, &fcdata, &fclen))
-			exit(1);
-	}
+	if (file_contexts && file_to_data(file_contexts, &fcdata, &fclen, argv[0]))
+		goto failure;
 
-	if (seusers) {
-		if (file_to_data(seusers, &seusersdata, &seuserslen))
-			exit(1);
-	}
+	if (seusers && file_to_data(seusers, &seusersdata, &seuserslen, argv[0]))
+		goto failure;
 
-	if (user_extra) {
-		if (file_to_data(user_extra, &user_extradata, &user_extralen))
-			exit(1);
-	}
+	if (user_extra && file_to_data(user_extra, &user_extradata, &user_extralen, argv[0]))
+		goto failure;
+
+	if (netfilter_contexts && file_to_data(netfilter_contexts, &ncdata, &nclen, argv[0]))
+		goto failure;
 
-	if (netfilter_contexts) {
-		if (file_to_data(netfilter_contexts, &ncdata, &nclen))
-			exit(1);
+	if (sepol_policy_file_create(&mod)) {
+		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+		goto failure;
 	}
 
-	if (file_to_policy_file(module, &mod, "r"))
-		exit(1);
+	fp = fopen(module, "re");
+	if (!fp) {
+		fprintf(stderr, "%s:  Could not open file %s:  %s\n", argv[0],
+			module, strerror(errno));
+		goto failure;
+	}
+	sepol_policy_file_set_fp(mod, fp);
 
 	if (sepol_module_package_create(&pkg)) {
 		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
-		exit(1);
+		goto failure;
 	}
 
 	if (sepol_policydb_read(sepol_module_package_get_policy(pkg), mod)) {
 		fprintf(stderr,
 			"%s:  Error while reading policy module from %s\n",
 			argv[0], module);
-		exit(1);
+		goto failure;
 	}
 
-	if (fclen)
-		sepol_module_package_set_file_contexts(pkg, fcdata, fclen);
+	fclose(fp);
+	fp = NULL;
 
-	if (seuserslen)
-		sepol_module_package_set_seusers(pkg, seusersdata, seuserslen);
+	if (fclen && sepol_module_package_set_file_contexts(pkg, fcdata, fclen)) {
+		fprintf(stderr, "%s:  Error while setting file contexts\n", argv[0]);
+		goto failure;
+	}
 
-	if (user_extra)
-		sepol_module_package_set_user_extra(pkg, user_extradata,
-						    user_extralen);
+	if (seuserslen && sepol_module_package_set_seusers(pkg, seusersdata, seuserslen)) {
+		fprintf(stderr, "%s:  Error while setting seusers\n", argv[0]);
+		goto failure;
+	}
 
-	if (nclen)
-		sepol_module_package_set_netfilter_contexts(pkg, ncdata, nclen);
+	if (user_extra && sepol_module_package_set_user_extra(pkg, user_extradata, user_extralen)) {
+		fprintf(stderr, "%s:  Error while setting extra users\n", argv[0]);
+		goto failure;
+	}
+
+	if (nclen && sepol_module_package_set_netfilter_contexts(pkg, ncdata, nclen)) {
+		fprintf(stderr, "%s:  Error while setting netfilter contexts\n", argv[0]);
+		goto failure;
+	}
 
-	if (file_to_policy_file(outfile, &out, "w"))
-		exit(1);
+	if (sepol_policy_file_create(&out)) {
+		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+		goto failure;
+	}
+
+	fp = fopen(outfile, "we");
+	if (!fp) {
+		fprintf(stderr, "%s:  Could not open file %s:  %s\n", argv[0],
+			outfile, strerror(errno));
+		goto failure;
+	}
+	sepol_policy_file_set_fp(out, fp);
 
 	if (sepol_module_package_write(pkg, out)) {
 		fprintf(stderr,
 			"%s:  Error while writing module package to %s\n",
 			argv[0], argv[1]);
-		exit(1);
+		goto failure;
 	}
 
-	if (fclen)
-		munmap(fcdata, fclen);
+	ret = fclose(fp);
+	fp = NULL;
+	if (ret) {
+		fprintf(stderr, "%s:  Could not close file %s:  %s\n", argv[0],
+			outfile, strerror(errno));
+		goto failure;
+	}
+
+	ret = EXIT_SUCCESS;
+	goto cleanup;
+
+failure:
+	ret = EXIT_FAILURE;
+
+cleanup:
+	if (fp)
+		fclose(fp);
+	sepol_policy_file_free(out);
 	if (nclen)
 		munmap(ncdata, nclen);
-	sepol_policy_file_free(mod);
-	sepol_policy_file_free(out);
+	if (user_extradata)
+		munmap(user_extradata, user_extralen);
+	if (seuserslen)
+		munmap(seusersdata, seuserslen);
+	if (fclen)
+		munmap(fcdata, fclen);
 	sepol_module_package_free(pkg);
-	free(file_contexts);
+	sepol_policy_file_free(mod);
+	free(netfilter_contexts);
+	free(user_extra);
+	free(seusers);
 	free(outfile);
+	free(file_contexts);
 	free(module);
-	free(seusers);
-	free(user_extra);
-	exit(0);
+
+	return ret;
 }
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 4/4] semodule_unpackage: update
  2023-07-06 14:53 [PATCH v2 1/4] semodule_expand: update Christian Göttsche
  2023-07-06 14:53 ` [PATCH v2 2/4] semodule_link: update Christian Göttsche
  2023-07-06 14:53 ` [PATCH v2 3/4] semodule_package: update Christian Göttsche
@ 2023-07-06 14:53 ` Christian Göttsche
  2023-07-19 15:58 ` [PATCH v2 1/4] semodule_expand: update James Carter
  3 siblings, 0 replies; 6+ messages in thread
From: Christian Göttsche @ 2023-07-06 14:53 UTC (permalink / raw)
  To: selinux

Drop unnecessary declarations.
Check closing file for incomplete write.
Rework resource cleanup, so that all files and allocated memory are
released in all branches, useful to minimize reports while debugging
libsepol under valgrind(8) or sanitizers.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
address comments from Jim
  * drop exit() calls
  * reduce to one final return statement
  * drop global variable progname
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 .../semodule_package/semodule_unpackage.c     | 129 +++++++++++-------
 1 file changed, 77 insertions(+), 52 deletions(-)

diff --git a/semodule-utils/semodule_package/semodule_unpackage.c b/semodule-utils/semodule_package/semodule_unpackage.c
index b8c4fbce..d5b84068 100644
--- a/semodule-utils/semodule_package/semodule_unpackage.c
+++ b/semodule-utils/semodule_package/semodule_unpackage.c
@@ -11,46 +11,23 @@
 #include <fcntl.h>
 #include <errno.h>
 
-char *progname = NULL;
-extern char *optarg;
-
-static __attribute__((__noreturn__)) void usage(void)
+static void usage(const char *progname)
 {
 	printf("usage: %s ppfile modfile [fcfile]\n", progname);
-	exit(1);
-}
-
-static int file_to_policy_file(const char *filename, struct sepol_policy_file **pf, const char *mode)
-{
-	FILE *f;
-
-	if (sepol_policy_file_create(pf)) {
-		fprintf(stderr, "%s:  Out of memory\n", progname);
-		return -1;
-	}
-
-	f = fopen(filename, mode);
-	if (!f) {
-		fprintf(stderr, "%s:  Could not open file %s:  %s\n", progname, strerror(errno), filename);
-		return -1;
-	}
-	sepol_policy_file_set_fp(*pf, f);
-	return 0;
 }
 
 int main(int argc, char **argv)
 {
-	struct sepol_module_package *pkg;
-	struct sepol_policy_file *in, *out;
-	FILE *fp;
+	struct sepol_module_package *pkg = NULL;
+	struct sepol_policy_file *in = NULL, *out = NULL;
+	FILE *fp = NULL;
 	size_t len;
-	char *ppfile, *modfile, *fcfile = NULL, *fcdata;
-
-	progname = argv[0];
+	const char *ppfile, *modfile, *fcfile = NULL, *fcdata;
+	int ret;
 
 	if (argc < 3) {
-		usage();
-		exit(1);
+		usage(argv[0]);
+		return EXIT_FAILURE;
 	}
 
 	ppfile = argv[1];
@@ -58,46 +35,94 @@ int main(int argc, char **argv)
 	if (argc >= 4)
 		fcfile = argv[3];
 
-	if (file_to_policy_file(ppfile, &in, "r"))
-		exit(1);
-
 	if (sepol_module_package_create(&pkg)) {
-                fprintf(stderr, "%s:  Out of memory\n", progname);
-                exit(1);
+		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+		goto failure;
 	}
 
-	if (sepol_module_package_read(pkg, in, 0) == -1) {
-                fprintf(stderr, "%s:  Error while reading policy module from %s\n",
-			progname, ppfile);
-                exit(1);
+	if (sepol_policy_file_create(&in)) {
+		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+		goto failure;
 	}
 
-	if (file_to_policy_file(modfile, &out, "w"))
-		exit(1);
+	fp = fopen(ppfile, "r");
+	if (!fp) {
+		fprintf(stderr, "%s:  Could not open file %s:  %s\n", argv[0], ppfile, strerror(errno));
+		goto failure;
+	}
+	sepol_policy_file_set_fp(in, fp);
 
-        if (sepol_policydb_write(sepol_module_package_get_policy(pkg), out)) {
-                fprintf(stderr, "%s:  Error while writing module to %s\n", progname, modfile);
-                exit(1);
-        }
+	if (sepol_module_package_read(pkg, in, 0) == -1) {
+		fprintf(stderr, "%s:  Error while reading policy module from %s\n",
+			argv[0], ppfile);
+		goto failure;
+	}
 
 	sepol_policy_file_free(in);
+	in = NULL;
+	fclose(fp);
+	fp = NULL;
+
+	if (sepol_policy_file_create(&out)) {
+		fprintf(stderr, "%s:  Out of memory\n", argv[0]);
+		goto failure;
+	}
+
+	fp = fopen(modfile, "w");
+	if (!fp) {
+		fprintf(stderr, "%s:  Could not open file %s:  %s\n", argv[0], modfile, strerror(errno));
+		goto failure;
+	}
+	sepol_policy_file_set_fp(out, fp);
+
+	if (sepol_policydb_write(sepol_module_package_get_policy(pkg), out)) {
+		fprintf(stderr, "%s:  Error while writing module to %s\n", argv[0], modfile);
+		goto failure;
+	}
+
+	ret = fclose(fp);
+	fp = NULL;
+	if (ret) {
+		fprintf(stderr, "%s:  Error while closing file %s:  %s\n", argv[0], modfile, strerror(errno));
+		goto failure;
+	}
+
 	sepol_policy_file_free(out);
+	out = NULL;
 
 	len = sepol_module_package_get_file_contexts_len(pkg);
 	if (fcfile && len) {
 		fp = fopen(fcfile, "w");
 		if (!fp) {
-			fprintf(stderr, "%s:  Could not open file %s:  %s\n", progname, strerror(errno), fcfile);
-			exit(1);
+			fprintf(stderr, "%s:  Could not open file %s:  %s\n", argv[0], fcfile, strerror(errno));
+			goto failure;
 		}
 		fcdata = sepol_module_package_get_file_contexts(pkg);
 		if (fwrite(fcdata, 1, len, fp) != len) {
-			fprintf(stderr, "%s:  Could not write file %s:  %s\n", progname, strerror(errno), fcfile);
-			exit(1);
+			fprintf(stderr, "%s:  Could not write file %s:  %s\n", argv[0], fcfile, strerror(errno));
+			goto failure;
+		}
+
+		ret = fclose(fp);
+		fp = NULL;
+		if (ret) {
+			fprintf(stderr, "%s:  Could not close file %s:  %s\n", argv[0], fcfile, strerror(errno));
+			goto failure;
 		}
-		fclose(fp);
 	}
 
+	ret = EXIT_SUCCESS;
+	goto cleanup;
+
+failure:
+	ret = EXIT_FAILURE;
+
+cleanup:
+	if (fp)
+		fclose(fp);
+	sepol_policy_file_free(out);
 	sepol_module_package_free(pkg);
-	exit(0);
+	sepol_policy_file_free(in);
+
+	return ret;
 }
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/4] semodule_expand: update
  2023-07-06 14:53 [PATCH v2 1/4] semodule_expand: update Christian Göttsche
                   ` (2 preceding siblings ...)
  2023-07-06 14:53 ` [PATCH v2 4/4] semodule_unpackage: update Christian Göttsche
@ 2023-07-19 15:58 ` James Carter
  2023-08-04 18:37   ` James Carter
  3 siblings, 1 reply; 6+ messages in thread
From: James Carter @ 2023-07-19 15:58 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: selinux

On Thu, Jul 6, 2023 at 10:54 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Drop unnecessary declarations.
> Reduce scope of file global variable.
> Mention -v argument in help usage message.
> More strict integer conversion.
> More strict argument count checking.
> Check closing file for incomplete write.
> Rework resource cleanup, so that all files and allocated memory are
> released in all branches, useful to minimize reports while debugging
> libsepol under valgrind(8) or sanitizers.
> Add help argument option -h.
> Set close-on-exec flag in case of any sibling threads.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For this series of four patches:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
> v2:
>   - address comments by Jim:
>     * drop exit() calls
>     * reduce to only one final return statement
>   - add help argument option -h
>   - set close-on-exec flag
> ---
>  .../semodule_expand/semodule_expand.8         |   5 +-
>  .../semodule_expand/semodule_expand.c         | 112 +++++++++++-------
>  2 files changed, 73 insertions(+), 44 deletions(-)
>
> diff --git a/semodule-utils/semodule_expand/semodule_expand.8 b/semodule-utils/semodule_expand/semodule_expand.8
> index 1b482a1f..84b943cd 100644
> --- a/semodule-utils/semodule_expand/semodule_expand.8
> +++ b/semodule-utils/semodule_expand/semodule_expand.8
> @@ -3,7 +3,7 @@
>  semodule_expand \- Expand a SELinux policy module package.
>
>  .SH SYNOPSIS
> -.B semodule_expand [-V ] [ -a ] [ -c [version]] basemodpkg outputfile
> +.B semodule_expand [-ahV] [ -c [version]] basemodpkg outputfile
>  .br
>  .SH DESCRIPTION
>  .PP
> @@ -17,6 +17,9 @@ together a set of packages into a single package).
>
>  .SH "OPTIONS"
>  .TP
> +.B \-h
> +show help
> +.TP
>  .B \-V
>  show version
>  .TP
> diff --git a/semodule-utils/semodule_expand/semodule_expand.c b/semodule-utils/semodule_expand/semodule_expand.c
> index 895cdd78..99380abe 100644
> --- a/semodule-utils/semodule_expand/semodule_expand.c
> +++ b/semodule-utils/semodule_expand/semodule_expand.c
> @@ -21,32 +21,25 @@
>  #include <unistd.h>
>  #include <string.h>
>
> -extern char *optarg;
> -extern int optind;
> -
> -int policyvers = 0;
> -
>  #define EXPANDPOLICY_VERSION "1.0"
>
> -static __attribute__((__noreturn__)) void usage(const char *program_name)
> +static void usage(const char *program_name)
>  {
> -       printf("usage: %s [-V -a -c [version]] basemodpkg outputfile\n",
> +       printf("usage: %s [-h -V -a -c [version] -v] basemodpkg outputfile\n",
>                program_name);
> -       exit(1);
>  }
>
>  int main(int argc, char **argv)
>  {
> -       char *basename, *outname;
> -       int ch, ret, show_version = 0, verbose = 0;
> -       struct sepol_policy_file *pf;
> -       sepol_module_package_t *base;
> -       sepol_policydb_t *out, *p;
> -       FILE *fp, *outfile;
> -       int check_assertions = 1;
> -       sepol_handle_t *handle;
> -
> -       while ((ch = getopt(argc, argv, "c:Vva")) != EOF) {
> +       const char *basename, *outname;
> +       int ch, ret, show_version = 0, verbose = 0, policyvers = 0, check_assertions = 1;
> +       struct sepol_policy_file *pf = NULL;
> +       sepol_module_package_t *base = NULL;
> +       sepol_policydb_t *out = NULL, *p;
> +       FILE *fp = NULL, *outfile = NULL;
> +       sepol_handle_t *handle = NULL;
> +
> +       while ((ch = getopt(argc, argv, "c:Vvah")) != EOF) {
>                 switch (ch) {
>                 case 'V':
>                         show_version = 1;
> @@ -54,14 +47,20 @@ int main(int argc, char **argv)
>                 case 'v':
>                         verbose = 1;
>                         break;
> +               case 'h':
> +                       usage(argv[0]);
> +                       return EXIT_SUCCESS;
>                 case 'c':{
> -                               long int n = strtol(optarg, NULL, 10);
> +                               long int n;
> +
> +                               errno = 0;
> +                               n = strtol(optarg, NULL, 10);
>                                 if (errno) {
>                                         fprintf(stderr,
>                                                 "%s:  Invalid policyvers specified: %s\n",
>                                                 argv[0], optarg);
>                                         usage(argv[0]);
> -                                       exit(1);
> +                                       return EXIT_FAILURE;
>                                 }
>                                 if (n < sepol_policy_kern_vers_min()
>                                     || n > sepol_policy_kern_vers_max()) {
> @@ -71,7 +70,7 @@ int main(int argc, char **argv)
>                                                 sepol_policy_kern_vers_min(),
>                                                 sepol_policy_kern_vers_max());
>                                         usage(argv[0]);
> -                                       exit(1);
> +                                       return EXIT_FAILURE;
>                                 }
>                                 policyvers = n;
>                                 break;
> @@ -82,6 +81,7 @@ int main(int argc, char **argv)
>                         }
>                 default:
>                         usage(argv[0]);
> +                       return EXIT_FAILURE;
>                 }
>         }
>
> @@ -92,84 +92,90 @@ int main(int argc, char **argv)
>
>         if (show_version) {
>                 printf("%s\n", EXPANDPOLICY_VERSION);
> -               exit(0);
> +               return EXIT_SUCCESS;
>         }
>
>         /* check args */
> -       if (argc < 3 || !(optind != (argc - 1))) {
> +       if (argc < 3 || argc - optind != 2) {
>                 fprintf(stderr,
>                         "%s:  You must provide the base module package and output filename\n",
>                         argv[0]);
>                 usage(argv[0]);
> +               return EXIT_FAILURE;
>         }
>
>         basename = argv[optind++];
>         outname = argv[optind];
>
>         handle = sepol_handle_create();
> -       if (!handle)
> -               exit(1);
> +       if (!handle) {
> +               fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> +               goto failure;
> +       }
>
>         if (sepol_policy_file_create(&pf)) {
>                 fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> -               exit(1);
> +               goto failure;
>         }
>
>         /* read the base module */
>         if (sepol_module_package_create(&base)) {
>                 fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> -               exit(1);
> +               goto failure;
>         }
> -       fp = fopen(basename, "r");
> +
> +       fp = fopen(basename, "re");
>         if (!fp) {
>                 fprintf(stderr, "%s:  Can't open '%s':  %s\n",
>                         argv[0], basename, strerror(errno));
> -               exit(1);
> +               goto failure;
>         }
> +
>         sepol_policy_file_set_fp(pf, fp);
>         ret = sepol_module_package_read(base, pf, 0);
>         if (ret) {
>                 fprintf(stderr, "%s:  Error in reading package from %s\n",
>                         argv[0], basename);
> -               exit(1);
> +               goto failure;
>         }
> +
>         fclose(fp);
> +       fp = NULL;
>
>         /* linking the base takes care of enabling optional avrules */
>         p = sepol_module_package_get_policy(base);
>         if (sepol_link_modules(handle, p, NULL, 0, 0)) {
>                 fprintf(stderr, "%s:  Error while enabling avrules\n", argv[0]);
> -               exit(1);
> +               goto failure;
>         }
>
>         /* create the output policy */
>
>         if (sepol_policydb_create(&out)) {
>                 fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> -               exit(1);
> +               goto failure;
>         }
>
>         sepol_set_expand_consume_base(handle, 1);
>
>         if (sepol_expand_module(handle, p, out, verbose, check_assertions)) {
>                 fprintf(stderr, "%s:  Error while expanding policy\n", argv[0]);
> -               exit(1);
> +               goto failure;
>         }
>
>         if (policyvers) {
>                 if (sepol_policydb_set_vers(out, policyvers)) {
>                         fprintf(stderr, "%s:  Invalid version %d\n", argv[0],
>                                 policyvers);
> -                       exit(1);
> +                       goto failure;
>                 }
>         }
>
> -       sepol_module_package_free(base);
> -
> -       outfile = fopen(outname, "w");
> +       outfile = fopen(outname, "we");
>         if (!outfile) {
> -               perror(outname);
> -               exit(1);
> +               fprintf(stderr, "%s:  Can't open '%s':  %s\n",
> +                       argv[0], outname, strerror(errno));
> +               goto failure;
>         }
>
>         sepol_policy_file_set_fp(pf, outfile);
> @@ -178,12 +184,32 @@ int main(int argc, char **argv)
>                 fprintf(stderr,
>                         "%s:  Error while writing expanded policy to %s\n",
>                         argv[0], outname);
> -               exit(1);
> +               goto failure;
>         }
> -       fclose(outfile);
> -       sepol_handle_destroy(handle);
> +
> +       ret = fclose(outfile);
> +       outfile = NULL;
> +       if (ret) {
> +               fprintf(stderr, "%s:  Error closing policy file %s:  %s\n",
> +                       argv[0], outname, strerror(errno));
> +               goto failure;
> +       }
> +
> +       ret = EXIT_SUCCESS;
> +       goto cleanup;
> +
> +failure:
> +       ret = EXIT_FAILURE;
> +
> +cleanup:
> +       if (outfile)
> +               fclose(outfile);
>         sepol_policydb_free(out);
> +       if (fp)
> +               fclose(fp);
> +       sepol_module_package_free(base);
>         sepol_policy_file_free(pf);
> +       sepol_handle_destroy(handle);
>
> -       return 0;
> +       return ret;
>  }
> --
> 2.40.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/4] semodule_expand: update
  2023-07-19 15:58 ` [PATCH v2 1/4] semodule_expand: update James Carter
@ 2023-08-04 18:37   ` James Carter
  0 siblings, 0 replies; 6+ messages in thread
From: James Carter @ 2023-08-04 18:37 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: selinux

On Wed, Jul 19, 2023 at 11:58 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Thu, Jul 6, 2023 at 10:54 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Drop unnecessary declarations.
> > Reduce scope of file global variable.
> > Mention -v argument in help usage message.
> > More strict integer conversion.
> > More strict argument count checking.
> > Check closing file for incomplete write.
> > Rework resource cleanup, so that all files and allocated memory are
> > released in all branches, useful to minimize reports while debugging
> > libsepol under valgrind(8) or sanitizers.
> > Add help argument option -h.
> > Set close-on-exec flag in case of any sibling threads.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For this series of four patches:
> Acked-by: James Carter <jwcart2@gmail.com>
>

This series of four patches has been merged.
Thanks,
Jim

> > ---
> > v2:
> >   - address comments by Jim:
> >     * drop exit() calls
> >     * reduce to only one final return statement
> >   - add help argument option -h
> >   - set close-on-exec flag
> > ---
> >  .../semodule_expand/semodule_expand.8         |   5 +-
> >  .../semodule_expand/semodule_expand.c         | 112 +++++++++++-------
> >  2 files changed, 73 insertions(+), 44 deletions(-)
> >
> > diff --git a/semodule-utils/semodule_expand/semodule_expand.8 b/semodule-utils/semodule_expand/semodule_expand.8
> > index 1b482a1f..84b943cd 100644
> > --- a/semodule-utils/semodule_expand/semodule_expand.8
> > +++ b/semodule-utils/semodule_expand/semodule_expand.8
> > @@ -3,7 +3,7 @@
> >  semodule_expand \- Expand a SELinux policy module package.
> >
> >  .SH SYNOPSIS
> > -.B semodule_expand [-V ] [ -a ] [ -c [version]] basemodpkg outputfile
> > +.B semodule_expand [-ahV] [ -c [version]] basemodpkg outputfile
> >  .br
> >  .SH DESCRIPTION
> >  .PP
> > @@ -17,6 +17,9 @@ together a set of packages into a single package).
> >
> >  .SH "OPTIONS"
> >  .TP
> > +.B \-h
> > +show help
> > +.TP
> >  .B \-V
> >  show version
> >  .TP
> > diff --git a/semodule-utils/semodule_expand/semodule_expand.c b/semodule-utils/semodule_expand/semodule_expand.c
> > index 895cdd78..99380abe 100644
> > --- a/semodule-utils/semodule_expand/semodule_expand.c
> > +++ b/semodule-utils/semodule_expand/semodule_expand.c
> > @@ -21,32 +21,25 @@
> >  #include <unistd.h>
> >  #include <string.h>
> >
> > -extern char *optarg;
> > -extern int optind;
> > -
> > -int policyvers = 0;
> > -
> >  #define EXPANDPOLICY_VERSION "1.0"
> >
> > -static __attribute__((__noreturn__)) void usage(const char *program_name)
> > +static void usage(const char *program_name)
> >  {
> > -       printf("usage: %s [-V -a -c [version]] basemodpkg outputfile\n",
> > +       printf("usage: %s [-h -V -a -c [version] -v] basemodpkg outputfile\n",
> >                program_name);
> > -       exit(1);
> >  }
> >
> >  int main(int argc, char **argv)
> >  {
> > -       char *basename, *outname;
> > -       int ch, ret, show_version = 0, verbose = 0;
> > -       struct sepol_policy_file *pf;
> > -       sepol_module_package_t *base;
> > -       sepol_policydb_t *out, *p;
> > -       FILE *fp, *outfile;
> > -       int check_assertions = 1;
> > -       sepol_handle_t *handle;
> > -
> > -       while ((ch = getopt(argc, argv, "c:Vva")) != EOF) {
> > +       const char *basename, *outname;
> > +       int ch, ret, show_version = 0, verbose = 0, policyvers = 0, check_assertions = 1;
> > +       struct sepol_policy_file *pf = NULL;
> > +       sepol_module_package_t *base = NULL;
> > +       sepol_policydb_t *out = NULL, *p;
> > +       FILE *fp = NULL, *outfile = NULL;
> > +       sepol_handle_t *handle = NULL;
> > +
> > +       while ((ch = getopt(argc, argv, "c:Vvah")) != EOF) {
> >                 switch (ch) {
> >                 case 'V':
> >                         show_version = 1;
> > @@ -54,14 +47,20 @@ int main(int argc, char **argv)
> >                 case 'v':
> >                         verbose = 1;
> >                         break;
> > +               case 'h':
> > +                       usage(argv[0]);
> > +                       return EXIT_SUCCESS;
> >                 case 'c':{
> > -                               long int n = strtol(optarg, NULL, 10);
> > +                               long int n;
> > +
> > +                               errno = 0;
> > +                               n = strtol(optarg, NULL, 10);
> >                                 if (errno) {
> >                                         fprintf(stderr,
> >                                                 "%s:  Invalid policyvers specified: %s\n",
> >                                                 argv[0], optarg);
> >                                         usage(argv[0]);
> > -                                       exit(1);
> > +                                       return EXIT_FAILURE;
> >                                 }
> >                                 if (n < sepol_policy_kern_vers_min()
> >                                     || n > sepol_policy_kern_vers_max()) {
> > @@ -71,7 +70,7 @@ int main(int argc, char **argv)
> >                                                 sepol_policy_kern_vers_min(),
> >                                                 sepol_policy_kern_vers_max());
> >                                         usage(argv[0]);
> > -                                       exit(1);
> > +                                       return EXIT_FAILURE;
> >                                 }
> >                                 policyvers = n;
> >                                 break;
> > @@ -82,6 +81,7 @@ int main(int argc, char **argv)
> >                         }
> >                 default:
> >                         usage(argv[0]);
> > +                       return EXIT_FAILURE;
> >                 }
> >         }
> >
> > @@ -92,84 +92,90 @@ int main(int argc, char **argv)
> >
> >         if (show_version) {
> >                 printf("%s\n", EXPANDPOLICY_VERSION);
> > -               exit(0);
> > +               return EXIT_SUCCESS;
> >         }
> >
> >         /* check args */
> > -       if (argc < 3 || !(optind != (argc - 1))) {
> > +       if (argc < 3 || argc - optind != 2) {
> >                 fprintf(stderr,
> >                         "%s:  You must provide the base module package and output filename\n",
> >                         argv[0]);
> >                 usage(argv[0]);
> > +               return EXIT_FAILURE;
> >         }
> >
> >         basename = argv[optind++];
> >         outname = argv[optind];
> >
> >         handle = sepol_handle_create();
> > -       if (!handle)
> > -               exit(1);
> > +       if (!handle) {
> > +               fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> > +               goto failure;
> > +       }
> >
> >         if (sepol_policy_file_create(&pf)) {
> >                 fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> > -               exit(1);
> > +               goto failure;
> >         }
> >
> >         /* read the base module */
> >         if (sepol_module_package_create(&base)) {
> >                 fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> > -               exit(1);
> > +               goto failure;
> >         }
> > -       fp = fopen(basename, "r");
> > +
> > +       fp = fopen(basename, "re");
> >         if (!fp) {
> >                 fprintf(stderr, "%s:  Can't open '%s':  %s\n",
> >                         argv[0], basename, strerror(errno));
> > -               exit(1);
> > +               goto failure;
> >         }
> > +
> >         sepol_policy_file_set_fp(pf, fp);
> >         ret = sepol_module_package_read(base, pf, 0);
> >         if (ret) {
> >                 fprintf(stderr, "%s:  Error in reading package from %s\n",
> >                         argv[0], basename);
> > -               exit(1);
> > +               goto failure;
> >         }
> > +
> >         fclose(fp);
> > +       fp = NULL;
> >
> >         /* linking the base takes care of enabling optional avrules */
> >         p = sepol_module_package_get_policy(base);
> >         if (sepol_link_modules(handle, p, NULL, 0, 0)) {
> >                 fprintf(stderr, "%s:  Error while enabling avrules\n", argv[0]);
> > -               exit(1);
> > +               goto failure;
> >         }
> >
> >         /* create the output policy */
> >
> >         if (sepol_policydb_create(&out)) {
> >                 fprintf(stderr, "%s:  Out of memory\n", argv[0]);
> > -               exit(1);
> > +               goto failure;
> >         }
> >
> >         sepol_set_expand_consume_base(handle, 1);
> >
> >         if (sepol_expand_module(handle, p, out, verbose, check_assertions)) {
> >                 fprintf(stderr, "%s:  Error while expanding policy\n", argv[0]);
> > -               exit(1);
> > +               goto failure;
> >         }
> >
> >         if (policyvers) {
> >                 if (sepol_policydb_set_vers(out, policyvers)) {
> >                         fprintf(stderr, "%s:  Invalid version %d\n", argv[0],
> >                                 policyvers);
> > -                       exit(1);
> > +                       goto failure;
> >                 }
> >         }
> >
> > -       sepol_module_package_free(base);
> > -
> > -       outfile = fopen(outname, "w");
> > +       outfile = fopen(outname, "we");
> >         if (!outfile) {
> > -               perror(outname);
> > -               exit(1);
> > +               fprintf(stderr, "%s:  Can't open '%s':  %s\n",
> > +                       argv[0], outname, strerror(errno));
> > +               goto failure;
> >         }
> >
> >         sepol_policy_file_set_fp(pf, outfile);
> > @@ -178,12 +184,32 @@ int main(int argc, char **argv)
> >                 fprintf(stderr,
> >                         "%s:  Error while writing expanded policy to %s\n",
> >                         argv[0], outname);
> > -               exit(1);
> > +               goto failure;
> >         }
> > -       fclose(outfile);
> > -       sepol_handle_destroy(handle);
> > +
> > +       ret = fclose(outfile);
> > +       outfile = NULL;
> > +       if (ret) {
> > +               fprintf(stderr, "%s:  Error closing policy file %s:  %s\n",
> > +                       argv[0], outname, strerror(errno));
> > +               goto failure;
> > +       }
> > +
> > +       ret = EXIT_SUCCESS;
> > +       goto cleanup;
> > +
> > +failure:
> > +       ret = EXIT_FAILURE;
> > +
> > +cleanup:
> > +       if (outfile)
> > +               fclose(outfile);
> >         sepol_policydb_free(out);
> > +       if (fp)
> > +               fclose(fp);
> > +       sepol_module_package_free(base);
> >         sepol_policy_file_free(pf);
> > +       sepol_handle_destroy(handle);
> >
> > -       return 0;
> > +       return ret;
> >  }
> > --
> > 2.40.1
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-08-04 18:38 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-06 14:53 [PATCH v2 1/4] semodule_expand: update Christian Göttsche
2023-07-06 14:53 ` [PATCH v2 2/4] semodule_link: update Christian Göttsche
2023-07-06 14:53 ` [PATCH v2 3/4] semodule_package: update Christian Göttsche
2023-07-06 14:53 ` [PATCH v2 4/4] semodule_unpackage: update Christian Göttsche
2023-07-19 15:58 ` [PATCH v2 1/4] semodule_expand: update James Carter
2023-08-04 18:37   ` James Carter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.