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;
>
> 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
>
