On Wed, Jul 19, 2023 at 11:58 AM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Thu, Jul 6, 2023 at 10:54 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.
> > Add help argument option -h.
> > Set close-on-exec flag in case of any sibling threads.
> >
> > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>
> For this series of four patches:
> Acked-by: James Carter <jwcart2@xxxxxxxxx>
>
This series of four patches has been merged.
Thanks,
Jim
> > ---
> > v2:
> > - address comments by Jim:
> > * drop exit() calls
> > * reduce to only one final return statement
> > - add help argument option -h
> > - set close-on-exec flag
> > ---
> > .../semodule_expand/semodule_expand.8 | 5 +-
> > .../semodule_expand/semodule_expand.c | 112 +++++++++++-------
> > 2 files changed, 73 insertions(+), 44 deletions(-)
> >
> > diff --git a/semodule-utils/semodule_expand/semodule_expand.8 b/semodule-utils/semodule_expand/semodule_expand.8
> > index 1b482a1f..84b943cd 100644
> > --- a/semodule-utils/semodule_expand/semodule_expand.8
> > +++ b/semodule-utils/semodule_expand/semodule_expand.8
> > @@ -3,7 +3,7 @@
> > semodule_expand \- Expand a SELinux policy module package.
> >
> > .SH SYNOPSIS
> > -.B semodule_expand [-V ] [ -a ] [ -c [version]] basemodpkg outputfile
> > +.B semodule_expand [-ahV] [ -c [version]] basemodpkg outputfile
> > .br
> > .SH DESCRIPTION
> > .PP
> > @@ -17,6 +17,9 @@ together a set of packages into a single package).
> >
> > .SH "OPTIONS"
> > .TP
> > +.B \-h
> > +show help
> > +.TP
> > .B \-V
> > show version
> > .TP
> > diff --git a/semodule-utils/semodule_expand/semodule_expand.c b/semodule-utils/semodule_expand/semodule_expand.c
> > index 895cdd78..99380abe 100644
> > --- a/semodule-utils/semodule_expand/semodule_expand.c
> > +++ b/semodule-utils/semodule_expand/semodule_expand.c
> > @@ -21,32 +21,25 @@
> > #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)
> > +static void usage(const char *program_name)
> > {
> > - printf("usage: %s [-V -a -c [version]] basemodpkg outputfile\n",
> > + printf("usage: %s [-h -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;
> > -
> > - while ((ch = getopt(argc, argv, "c:Vva")) != EOF) {
> > + 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:Vvah")) != EOF) {
> > switch (ch) {
> > case 'V':
> > show_version = 1;
> > @@ -54,14 +47,20 @@ int main(int argc, char **argv)
> > case 'v':
> > verbose = 1;
> > break;
> > + case 'h':
> > + usage(argv[0]);
> > + return EXIT_SUCCESS;
> > 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);
> > + return EXIT_FAILURE;
> > }
> > if (n < sepol_policy_kern_vers_min()
> > || n > sepol_policy_kern_vers_max()) {
> > @@ -71,7 +70,7 @@ int main(int argc, char **argv)
> > sepol_policy_kern_vers_min(),
> > sepol_policy_kern_vers_max());
> > usage(argv[0]);
> > - exit(1);
> > + return EXIT_FAILURE;
> > }
> > policyvers = n;
> > break;
> > @@ -82,6 +81,7 @@ int main(int argc, char **argv)
> > }
> > default:
> > usage(argv[0]);
> > + return EXIT_FAILURE;
> > }
> > }
> >
> > @@ -92,84 +92,90 @@ int main(int argc, char **argv)
> >
> > if (show_version) {
> > printf("%s\n", EXPANDPOLICY_VERSION);
> > - exit(0);
> > + return EXIT_SUCCESS;
> > }
> >
> > /* 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]);
> > usage(argv[0]);
> > + return EXIT_FAILURE;
> > }
> >
> > basename = argv[optind++];
> > 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");
> > +
> > + fp = fopen(basename, "re");
> > 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");
> > + outfile = fopen(outname, "we");
> > 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 +184,32 @@ 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;
> > + goto cleanup;
> > +
> > +failure:
> > + ret = EXIT_FAILURE;
> > +
> > +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 0;
> > + return ret;
> > }
> > --
> > 2.40.1
> >
