| Message ID | 20230512110730.78672-2-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 63e798a2034a |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [RFC,1/4] semodule_expand: update | expand |
On Fri, May 12, 2023 at 7:22 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > 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. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > semodule-utils/semodule_link/semodule_link.c | 65 ++++++++++++-------- > 1 file changed, 38 insertions(+), 27 deletions(-) > > diff --git a/semodule-utils/semodule_link/semodule_link.c b/semodule-utils/semodule_link/semodule_link.c > index 38a6843c..58a82cb0 100644 > --- a/semodule-utils/semodule_link/semodule_link.c > +++ b/semodule-utils/semodule_link/semodule_link.c > @@ -21,9 +21,7 @@ > > #define LINKPOLICY_VERSION "1.0" > > -char *progname; > -extern char *optarg; > -extern int optind; > +static const char *progname; > Is there a reason that we can't eliminate this global and just pass in the program name? > static __attribute__((__noreturn__)) void usage(const char *program_name) > { > @@ -32,7 +30,7 @@ static __attribute__((__noreturn__)) void usage(const char *program_name) > exit(1); Same comment as for patch 1 about "exit(1)" > } > > -static sepol_module_package_t *load_module(char *filename) > +static sepol_module_package_t *load_module(const char *filename) > { > int ret; > FILE *fp = NULL; > @@ -49,7 +47,7 @@ static sepol_module_package_t *load_module(char *filename) > } > fp = fopen(filename, "r"); > 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,11 +74,10 @@ 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; > + 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; > > progname = argv[0]; > > @@ -106,7 +103,7 @@ int main(int argc, char **argv) > } > > /* 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]); > @@ -119,18 +116,15 @@ int main(int argc, char **argv) > 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]); > @@ -138,39 +132,56 @@ int main(int argc, char **argv) > 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, "w"); > if (!outfile) { > - perror(outname); > - exit(1); > + fprintf(stderr, "%s: Could not open output file %s: %s\n", > + progname, 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; > + > +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; > + > +failure: > + ret = EXIT_FAILURE; > + goto cleanup; Same comment as for patch 1 here as well. Thanks, Jim > } > -- > 2.40.1 >
diff --git a/semodule-utils/semodule_link/semodule_link.c b/semodule-utils/semodule_link/semodule_link.c index 38a6843c..58a82cb0 100644 --- a/semodule-utils/semodule_link/semodule_link.c +++ b/semodule-utils/semodule_link/semodule_link.c @@ -21,9 +21,7 @@ #define LINKPOLICY_VERSION "1.0" -char *progname; -extern char *optarg; -extern int optind; +static const char *progname; static __attribute__((__noreturn__)) void usage(const char *program_name) { @@ -32,7 +30,7 @@ static __attribute__((__noreturn__)) void usage(const char *program_name) exit(1); } -static sepol_module_package_t *load_module(char *filename) +static sepol_module_package_t *load_module(const char *filename) { int ret; FILE *fp = NULL; @@ -49,7 +47,7 @@ static sepol_module_package_t *load_module(char *filename) } fp = fopen(filename, "r"); 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,11 +74,10 @@ 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; + 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; progname = argv[0]; @@ -106,7 +103,7 @@ int main(int argc, char **argv) } /* 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]); @@ -119,18 +116,15 @@ int main(int argc, char **argv) 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]); @@ -138,39 +132,56 @@ int main(int argc, char **argv) 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, "w"); if (!outfile) { - perror(outname); - exit(1); + fprintf(stderr, "%s: Could not open output file %s: %s\n", + progname, 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; + +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; + +failure: + ret = EXIT_FAILURE; + goto cleanup; }
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. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- semodule-utils/semodule_link/semodule_link.c | 65 ++++++++++++-------- 1 file changed, 38 insertions(+), 27 deletions(-)