| Message ID | 20230512110730.78672-3-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f40d4f3ddab0 |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [RFC,1/4] semodule_expand: update | expand |
On Fri, May 12, 2023 at 7:25 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > 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. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > .../semodule_package/semodule_package.c | 203 +++++++++++------- > 1 file changed, 125 insertions(+), 78 deletions(-) > > diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c > index bc8584b5..7485e254 100644 > --- a/semodule-utils/semodule_package/semodule_package.c > +++ b/semodule-utils/semodule_package/semodule_package.c > @@ -19,8 +19,7 @@ > #include <fcntl.h> > #include <errno.h> > > -char *progname = NULL; > -extern char *optarg; > +static const char *progname = NULL; > > static __attribute__((__noreturn__)) void usage(const char *prog) > { > @@ -37,26 +36,6 @@ static __attribute__((__noreturn__)) void usage(const char *prog) > exit(1); Same comment as patch 1 about "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; > -} > - > static int file_to_data(const char *path, char **data, size_t * len) > { > int fd; > @@ -94,17 +73,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'}, > @@ -115,11 +95,12 @@ int main(int argc, char **argv) > {NULL, 0, NULL, 0} > }; > > + progname = argv[0]; > + > while ((i = getopt_long(argc, argv, "m:f:s:u:o:n:h", opts, NULL)) != -1) { > switch (i) { > case 'h': > - usage(argv[0]); > - exit(0); > + usage(progname); > case 'm': > if (module) { > fprintf(stderr, > @@ -127,8 +108,10 @@ int main(int argc, char **argv) > exit(1); > } > module = strdup(optarg); > - if (!module) > + if (!module) { > + fprintf(stderr, "%s: Out of memory\n", progname); > exit(1); > + } > break; > case 'f': > if (file_contexts) { > @@ -137,8 +120,10 @@ int main(int argc, char **argv) > exit(1); > } > file_contexts = strdup(optarg); > - if (!file_contexts) > + if (!file_contexts) { > + fprintf(stderr, "%s: Out of memory\n", progname); > exit(1); > + } > break; > case 'o': > if (outfile) { > @@ -147,8 +132,10 @@ int main(int argc, char **argv) > exit(1); > } > outfile = strdup(optarg); > - if (!outfile) > + if (!outfile) { > + fprintf(stderr, "%s: Out of memory\n", progname); > exit(1); > + } > break; > case 's': > if (seusers) { > @@ -157,8 +144,10 @@ int main(int argc, char **argv) > exit(1); > } > seusers = strdup(optarg); > - if (!seusers) > + if (!seusers) { > + fprintf(stderr, "%s: Out of memory\n", progname); > exit(1); > + } > break; > case 'u': > if (user_extra) { > @@ -167,8 +156,10 @@ int main(int argc, char **argv) > exit(1); > } > user_extra = strdup(optarg); > - if (!user_extra) > + if (!user_extra) { > + fprintf(stderr, "%s: Out of memory\n", progname); > exit(1); > + } > break; > case 'n': > if (netfilter_contexts) { > @@ -177,88 +168,144 @@ int main(int argc, char **argv) > exit(1); > } > netfilter_contexts = strdup(optarg); > - if (!netfilter_contexts) > + if (!netfilter_contexts) { > + fprintf(stderr, "%s: Out of memory\n", progname); > exit(1); > + } > break; > + case '?': > + usage(progname); What is this option "?" for? > + default: > + fprintf(stderr, "%s: Unsupported getopt return code: %d\n", progname, i); > + usage(progname); Why not just default to calling usage() without the error message? > } > } > > - progname = argv[0]; > - > - if (!module || !outfile) { > - usage(argv[0]); > - exit(0); > + if (optind < argc) { > + fprintf(stderr, "%s: Superfluous command line arguments: ", progname); > + while (optind < argc) > + fprintf(stderr, "%s ", argv[optind++]); > + fprintf(stderr, "\n"); > + usage(progname); > } > > - if (file_contexts) { > - if (file_to_data(file_contexts, &fcdata, &fclen)) > - exit(1); > - } > + if (!module || !outfile) > + usage(progname); > > - if (seusers) { > - if (file_to_data(seusers, &seusersdata, &seuserslen)) > - exit(1); > - } > + if (file_contexts && file_to_data(file_contexts, &fcdata, &fclen)) > + goto failure; > > - if (user_extra) { > - if (file_to_data(user_extra, &user_extradata, &user_extralen)) > - exit(1); > - } > + if (seusers && file_to_data(seusers, &seusersdata, &seuserslen)) > + goto failure; > + > + if (user_extra && file_to_data(user_extra, &user_extradata, &user_extralen)) > + goto failure; > > - if (netfilter_contexts) { > - if (file_to_data(netfilter_contexts, &ncdata, &nclen)) > - exit(1); > + if (netfilter_contexts && file_to_data(netfilter_contexts, &ncdata, &nclen)) > + goto failure; > + > + if (sepol_policy_file_create(&mod)) { > + fprintf(stderr, "%s: Out of memory\n", progname); > + goto failure; > } > > - if (file_to_policy_file(module, &mod, "r")) > - exit(1); > + fp = fopen(module, "r"); > + if (!fp) { > + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, > + 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", progname); > + 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", progname); > + 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", progname); > + goto failure; > + } > + > + if (nclen && sepol_module_package_set_netfilter_contexts(pkg, ncdata, nclen)) { > + fprintf(stderr, "%s: Error while setting netfilter contexts\n", progname); > + goto failure; > + } > + > + if (sepol_policy_file_create(&out)) { > + fprintf(stderr, "%s: Out of memory\n", progname); > + goto failure; > + } > > - if (file_to_policy_file(outfile, &out, "w")) > - exit(1); > + fp = fopen(outfile, "w"); > + if (!fp) { > + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, > + 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", progname, > + outfile, strerror(errno)); > + goto failure; > + } > + > + ret = EXIT_SUCCESS; > + > +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; > + > +failure: > + ret = EXIT_FAILURE; > + goto cleanup; Same as the comment for patch 1 about preferring "return ret" to be at the end. Thanks, Jim > } > -- > 2.40.1 >
On Fri, Jun 9, 2023 at 3:32 PM James Carter <jwcart2@gmail.com> wrote: > > On Fri, May 12, 2023 at 7:25 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > 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. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > .../semodule_package/semodule_package.c | 203 +++++++++++------- > > 1 file changed, 125 insertions(+), 78 deletions(-) > > > > diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c > > index bc8584b5..7485e254 100644 > > --- a/semodule-utils/semodule_package/semodule_package.c > > +++ b/semodule-utils/semodule_package/semodule_package.c > > @@ -19,8 +19,7 @@ > > #include <fcntl.h> > > #include <errno.h> > > > > -char *progname = NULL; > > -extern char *optarg; > > +static const char *progname = NULL; I forgot to ask if this global can be removed as well. Thanks, Jim > > > > static __attribute__((__noreturn__)) void usage(const char *prog) > > { > > @@ -37,26 +36,6 @@ static __attribute__((__noreturn__)) void usage(const char *prog) > > exit(1); > > Same comment as patch 1 about "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; > > -} > > - > > static int file_to_data(const char *path, char **data, size_t * len) > > { > > int fd; > > @@ -94,17 +73,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'}, > > @@ -115,11 +95,12 @@ int main(int argc, char **argv) > > {NULL, 0, NULL, 0} > > }; > > > > + progname = argv[0]; > > + > > while ((i = getopt_long(argc, argv, "m:f:s:u:o:n:h", opts, NULL)) != -1) { > > switch (i) { > > case 'h': > > - usage(argv[0]); > > - exit(0); > > + usage(progname); > > case 'm': > > if (module) { > > fprintf(stderr, > > @@ -127,8 +108,10 @@ int main(int argc, char **argv) > > exit(1); > > } > > module = strdup(optarg); > > - if (!module) > > + if (!module) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > exit(1); > > + } > > break; > > case 'f': > > if (file_contexts) { > > @@ -137,8 +120,10 @@ int main(int argc, char **argv) > > exit(1); > > } > > file_contexts = strdup(optarg); > > - if (!file_contexts) > > + if (!file_contexts) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > exit(1); > > + } > > break; > > case 'o': > > if (outfile) { > > @@ -147,8 +132,10 @@ int main(int argc, char **argv) > > exit(1); > > } > > outfile = strdup(optarg); > > - if (!outfile) > > + if (!outfile) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > exit(1); > > + } > > break; > > case 's': > > if (seusers) { > > @@ -157,8 +144,10 @@ int main(int argc, char **argv) > > exit(1); > > } > > seusers = strdup(optarg); > > - if (!seusers) > > + if (!seusers) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > exit(1); > > + } > > break; > > case 'u': > > if (user_extra) { > > @@ -167,8 +156,10 @@ int main(int argc, char **argv) > > exit(1); > > } > > user_extra = strdup(optarg); > > - if (!user_extra) > > + if (!user_extra) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > exit(1); > > + } > > break; > > case 'n': > > if (netfilter_contexts) { > > @@ -177,88 +168,144 @@ int main(int argc, char **argv) > > exit(1); > > } > > netfilter_contexts = strdup(optarg); > > - if (!netfilter_contexts) > > + if (!netfilter_contexts) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > exit(1); > > + } > > break; > > + case '?': > > + usage(progname); > > What is this option "?" for? > > > + default: > > + fprintf(stderr, "%s: Unsupported getopt return code: %d\n", progname, i); > > + usage(progname); > > Why not just default to calling usage() without the error message? > > > } > > } > > > > - progname = argv[0]; > > - > > - if (!module || !outfile) { > > - usage(argv[0]); > > - exit(0); > > + if (optind < argc) { > > + fprintf(stderr, "%s: Superfluous command line arguments: ", progname); > > + while (optind < argc) > > + fprintf(stderr, "%s ", argv[optind++]); > > + fprintf(stderr, "\n"); > > + usage(progname); > > } > > > > - if (file_contexts) { > > - if (file_to_data(file_contexts, &fcdata, &fclen)) > > - exit(1); > > - } > > + if (!module || !outfile) > > + usage(progname); > > > > - if (seusers) { > > - if (file_to_data(seusers, &seusersdata, &seuserslen)) > > - exit(1); > > - } > > + if (file_contexts && file_to_data(file_contexts, &fcdata, &fclen)) > > + goto failure; > > > > - if (user_extra) { > > - if (file_to_data(user_extra, &user_extradata, &user_extralen)) > > - exit(1); > > - } > > + if (seusers && file_to_data(seusers, &seusersdata, &seuserslen)) > > + goto failure; > > + > > + if (user_extra && file_to_data(user_extra, &user_extradata, &user_extralen)) > > + goto failure; > > > > - if (netfilter_contexts) { > > - if (file_to_data(netfilter_contexts, &ncdata, &nclen)) > > - exit(1); > > + if (netfilter_contexts && file_to_data(netfilter_contexts, &ncdata, &nclen)) > > + goto failure; > > + > > + if (sepol_policy_file_create(&mod)) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > + goto failure; > > } > > > > - if (file_to_policy_file(module, &mod, "r")) > > - exit(1); > > + fp = fopen(module, "r"); > > + if (!fp) { > > + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, > > + 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", progname); > > + 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", progname); > > + 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", progname); > > + goto failure; > > + } > > + > > + if (nclen && sepol_module_package_set_netfilter_contexts(pkg, ncdata, nclen)) { > > + fprintf(stderr, "%s: Error while setting netfilter contexts\n", progname); > > + goto failure; > > + } > > + > > + if (sepol_policy_file_create(&out)) { > > + fprintf(stderr, "%s: Out of memory\n", progname); > > + goto failure; > > + } > > > > - if (file_to_policy_file(outfile, &out, "w")) > > - exit(1); > > + fp = fopen(outfile, "w"); > > + if (!fp) { > > + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, > > + 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", progname, > > + outfile, strerror(errno)); > > + goto failure; > > + } > > + > > + ret = EXIT_SUCCESS; > > + > > +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; > > + > > +failure: > > + ret = EXIT_FAILURE; > > + goto cleanup; > > Same as the comment for patch 1 about preferring "return ret" to be at the end. > > Thanks, > Jim > > > } > > -- > > 2.40.1 > >
diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c index bc8584b5..7485e254 100644 --- a/semodule-utils/semodule_package/semodule_package.c +++ b/semodule-utils/semodule_package/semodule_package.c @@ -19,8 +19,7 @@ #include <fcntl.h> #include <errno.h> -char *progname = NULL; -extern char *optarg; +static const char *progname = NULL; static __attribute__((__noreturn__)) void usage(const char *prog) { @@ -37,26 +36,6 @@ static __attribute__((__noreturn__)) void usage(const char *prog) 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; -} - static int file_to_data(const char *path, char **data, size_t * len) { int fd; @@ -94,17 +73,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'}, @@ -115,11 +95,12 @@ int main(int argc, char **argv) {NULL, 0, NULL, 0} }; + progname = argv[0]; + while ((i = getopt_long(argc, argv, "m:f:s:u:o:n:h", opts, NULL)) != -1) { switch (i) { case 'h': - usage(argv[0]); - exit(0); + usage(progname); case 'm': if (module) { fprintf(stderr, @@ -127,8 +108,10 @@ int main(int argc, char **argv) exit(1); } module = strdup(optarg); - if (!module) + if (!module) { + fprintf(stderr, "%s: Out of memory\n", progname); exit(1); + } break; case 'f': if (file_contexts) { @@ -137,8 +120,10 @@ int main(int argc, char **argv) exit(1); } file_contexts = strdup(optarg); - if (!file_contexts) + if (!file_contexts) { + fprintf(stderr, "%s: Out of memory\n", progname); exit(1); + } break; case 'o': if (outfile) { @@ -147,8 +132,10 @@ int main(int argc, char **argv) exit(1); } outfile = strdup(optarg); - if (!outfile) + if (!outfile) { + fprintf(stderr, "%s: Out of memory\n", progname); exit(1); + } break; case 's': if (seusers) { @@ -157,8 +144,10 @@ int main(int argc, char **argv) exit(1); } seusers = strdup(optarg); - if (!seusers) + if (!seusers) { + fprintf(stderr, "%s: Out of memory\n", progname); exit(1); + } break; case 'u': if (user_extra) { @@ -167,8 +156,10 @@ int main(int argc, char **argv) exit(1); } user_extra = strdup(optarg); - if (!user_extra) + if (!user_extra) { + fprintf(stderr, "%s: Out of memory\n", progname); exit(1); + } break; case 'n': if (netfilter_contexts) { @@ -177,88 +168,144 @@ int main(int argc, char **argv) exit(1); } netfilter_contexts = strdup(optarg); - if (!netfilter_contexts) + if (!netfilter_contexts) { + fprintf(stderr, "%s: Out of memory\n", progname); exit(1); + } break; + case '?': + usage(progname); + default: + fprintf(stderr, "%s: Unsupported getopt return code: %d\n", progname, i); + usage(progname); } } - progname = argv[0]; - - if (!module || !outfile) { - usage(argv[0]); - exit(0); + if (optind < argc) { + fprintf(stderr, "%s: Superfluous command line arguments: ", progname); + while (optind < argc) + fprintf(stderr, "%s ", argv[optind++]); + fprintf(stderr, "\n"); + usage(progname); } - if (file_contexts) { - if (file_to_data(file_contexts, &fcdata, &fclen)) - exit(1); - } + if (!module || !outfile) + usage(progname); - if (seusers) { - if (file_to_data(seusers, &seusersdata, &seuserslen)) - exit(1); - } + if (file_contexts && file_to_data(file_contexts, &fcdata, &fclen)) + goto failure; - if (user_extra) { - if (file_to_data(user_extra, &user_extradata, &user_extralen)) - exit(1); - } + if (seusers && file_to_data(seusers, &seusersdata, &seuserslen)) + goto failure; + + if (user_extra && file_to_data(user_extra, &user_extradata, &user_extralen)) + goto failure; - if (netfilter_contexts) { - if (file_to_data(netfilter_contexts, &ncdata, &nclen)) - exit(1); + if (netfilter_contexts && file_to_data(netfilter_contexts, &ncdata, &nclen)) + goto failure; + + if (sepol_policy_file_create(&mod)) { + fprintf(stderr, "%s: Out of memory\n", progname); + goto failure; } - if (file_to_policy_file(module, &mod, "r")) - exit(1); + fp = fopen(module, "r"); + if (!fp) { + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, + 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", progname); + 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", progname); + 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", progname); + goto failure; + } + + if (nclen && sepol_module_package_set_netfilter_contexts(pkg, ncdata, nclen)) { + fprintf(stderr, "%s: Error while setting netfilter contexts\n", progname); + goto failure; + } + + if (sepol_policy_file_create(&out)) { + fprintf(stderr, "%s: Out of memory\n", progname); + goto failure; + } - if (file_to_policy_file(outfile, &out, "w")) - exit(1); + fp = fopen(outfile, "w"); + if (!fp) { + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, + 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", progname, + outfile, strerror(errno)); + goto failure; + } + + ret = EXIT_SUCCESS; + +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; + +failure: + ret = EXIT_FAILURE; + goto cleanup; }
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. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- .../semodule_package/semodule_package.c | 203 +++++++++++------- 1 file changed, 125 insertions(+), 78 deletions(-)