| Message ID | 20230512110730.78672-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c52ff76180d0 |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [RFC,1/4] semodule_expand: update | expand |
On Fri, May 12, 2023 at 7:28 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. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > .../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 >
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); } 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; }
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@googlemail.com> --- .../semodule_expand/semodule_expand.c | 91 +++++++++++-------- 1 file changed, 55 insertions(+), 36 deletions(-)