On Fri, Jun 9, 2023 at 3:32 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Fri, May 12, 2023 at 7:25 AM Christian Göttsche
> <cgzones@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > .../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
> >
