Re: [RFC PATCH 1/4] semodule_expand: update

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, May 12, 2023 at 7:28 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> 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.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
>  .../semodule_expand/semodule_expand.c         | 91 +++++++++++--------
>  1 file changed, 55 insertions(+), 36 deletions(-)
>
> diff --git a/semodule-utils/semodule_expand/semodule_expand.c b/semodule-utils/semodule_expand/semodule_expand.c
> index 895cdd78..8d9feb05 100644
> --- a/semodule-utils/semodule_expand/semodule_expand.c
> +++ b/semodule-utils/semodule_expand/semodule_expand.c
> @@ -21,30 +21,24 @@
>  #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)
>  {
> -       printf("usage: %s [-V -a -c [version]] basemodpkg outputfile\n",
> +       printf("usage: %s [-V -a -c [version] -v] basemodpkg outputfile\n",
>                program_name);
>         exit(1);

If you are going to use EXIT_FAILURE, then all the uses of "exit(1)"
should be removed.

>  }
>
>  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;
> +       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:Vva")) != EOF) {
>                 switch (ch) {
> @@ -55,13 +49,15 @@ int main(int argc, char **argv)
>                         verbose = 1;
>                         break;
>                 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);
>                                 }
>                                 if (n < sepol_policy_kern_vers_min()
>                                     || n > sepol_policy_kern_vers_max()) {
> @@ -71,7 +67,6 @@ int main(int argc, char **argv)
>                                                 sepol_policy_kern_vers_min(),
>                                                 sepol_policy_kern_vers_max());
>                                         usage(argv[0]);
> -                                       exit(1);
>                                 }
>                                 policyvers = n;
>                                 break;
> @@ -96,7 +91,7 @@ int main(int argc, char **argv)
>         }
>
>         /* 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]);
> @@ -107,69 +102,74 @@ int main(int argc, char **argv)
>         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");
>         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");
>         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 +178,31 @@ 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;
> +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 ret;
>
> -       return 0;
> +failure:
> +       ret = EXIT_FAILURE;
> +       goto cleanup;
>  }

I would prefer to have the return at the end.
Something like this:

  ret = EXIT_SUCCESS;
  goto cleanup;
failure:
  ret = EXIT_FAILURE;
cleanup:
...
  return ret;
}

Thanks,
Jim

> --
> 2.40.1
>




[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux