On Fri, May 12, 2023 at 7:22 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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
>
