diff mbox series

[v2,1/4] semodule_expand: update

Message ID 20230706145335.71452-1-cgzones@googlemail.com (mailing list archive)
State Accepted
Commit c52ff76180d0
Headers show
Series [v2,1/4] semodule_expand: update | expand

Commit Message

Christian Göttsche July 6, 2023, 2:53 p.m. UTC
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(-)

Comments

James Carter July 19, 2023, 3:58 p.m. UTC | #1
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
>
James Carter Aug. 4, 2023, 6:37 p.m. UTC | #2
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
> >
diff mbox series

Patch

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;
 }