On Fri, May 12, 2023 at 7:22 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> Drop unnecessary declarations.
> 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_unpackage.c | 122 +++++++++++-------
> 1 file changed, 75 insertions(+), 47 deletions(-)
>
> diff --git a/semodule-utils/semodule_package/semodule_unpackage.c b/semodule-utils/semodule_package/semodule_unpackage.c
> index b8c4fbce..21c97953 100644
> --- a/semodule-utils/semodule_package/semodule_unpackage.c
> +++ b/semodule-utils/semodule_package/semodule_unpackage.c
> @@ -11,8 +11,7 @@
> #include <fcntl.h>
> #include <errno.h>
>
> -char *progname = NULL;
> -extern char *optarg;
> +static const char *progname = NULL;
Can we get rid of the global here and just pass in the program name?
>
> static __attribute__((__noreturn__)) void usage(void)
> {
> @@ -20,84 +19,113 @@ static __attribute__((__noreturn__)) void usage(void)
> exit(1);
Same comment as patch 1 about removing "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;
> -}
> -
> int main(int argc, char **argv)
> {
> - struct sepol_module_package *pkg;
> - struct sepol_policy_file *in, *out;
> - FILE *fp;
> + struct sepol_module_package *pkg = NULL;
> + struct sepol_policy_file *in = NULL, *out = NULL;
> + FILE *fp = NULL;
> size_t len;
> - char *ppfile, *modfile, *fcfile = NULL, *fcdata;
> + const char *ppfile, *modfile, *fcfile = NULL, *fcdata;
> + int ret;
>
> progname = argv[0];
>
> - if (argc < 3) {
> + if (argc < 3)
> usage();
> - exit(1);
> - }
>
> ppfile = argv[1];
> modfile = argv[2];
> if (argc >= 4)
> fcfile = argv[3];
>
> - if (file_to_policy_file(ppfile, &in, "r"))
> - exit(1);
> -
> if (sepol_module_package_create(&pkg)) {
> - fprintf(stderr, "%s: Out of memory\n", progname);
> - exit(1);
> + fprintf(stderr, "%s: Out of memory\n", progname);
> + goto failure;
> + }
> +
> + if (sepol_policy_file_create(&in)) {
> + fprintf(stderr, "%s: Out of memory\n", progname);
> + goto failure;
> }
>
> + fp = fopen(ppfile, "r");
> + if (!fp) {
> + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, ppfile, strerror(errno));
> + goto failure;
> + }
> + sepol_policy_file_set_fp(in, fp);
> +
> if (sepol_module_package_read(pkg, in, 0) == -1) {
> - fprintf(stderr, "%s: Error while reading policy module from %s\n",
> + fprintf(stderr, "%s: Error while reading policy module from %s\n",
> progname, ppfile);
> - exit(1);
> + goto failure;
> }
>
> - if (file_to_policy_file(modfile, &out, "w"))
> - exit(1);
> + sepol_policy_file_free(in);
> + in = NULL;
> + fclose(fp);
> + fp = NULL;
>
> - if (sepol_policydb_write(sepol_module_package_get_policy(pkg), out)) {
> - fprintf(stderr, "%s: Error while writing module to %s\n", progname, modfile);
> - exit(1);
> - }
> + if (sepol_policy_file_create(&out)) {
> + fprintf(stderr, "%s: Out of memory\n", progname);
> + goto failure;
> + }
> +
> + fp = fopen(modfile, "w");
> + if (!fp) {
> + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, modfile, strerror(errno));
> + goto failure;
> + }
> + sepol_policy_file_set_fp(out, fp);
> +
> + if (sepol_policydb_write(sepol_module_package_get_policy(pkg), out)) {
> + fprintf(stderr, "%s: Error while writing module to %s\n", progname, modfile);
> + goto failure;
> + }
> +
> + ret = fclose(fp);
> + fp = NULL;
> + if (ret) {
> + fprintf(stderr, "%s: Error while closing file %s: %s\n", progname, modfile, strerror(errno));
> + goto failure;
> + }
>
> - sepol_policy_file_free(in);
> sepol_policy_file_free(out);
> + out = NULL;
>
> len = sepol_module_package_get_file_contexts_len(pkg);
> if (fcfile && len) {
> fp = fopen(fcfile, "w");
> if (!fp) {
> - fprintf(stderr, "%s: Could not open file %s: %s\n", progname, strerror(errno), fcfile);
> - exit(1);
> + fprintf(stderr, "%s: Could not open file %s: %s\n", progname, fcfile, strerror(errno));
> + goto failure;
> }
> fcdata = sepol_module_package_get_file_contexts(pkg);
> if (fwrite(fcdata, 1, len, fp) != len) {
> - fprintf(stderr, "%s: Could not write file %s: %s\n", progname, strerror(errno), fcfile);
> - exit(1);
> + fprintf(stderr, "%s: Could not write file %s: %s\n", progname, fcfile, strerror(errno));
> + goto failure;
> + }
> +
> + ret = fclose(fp);
> + fp = NULL;
> + if (ret) {
> + fprintf(stderr, "%s: Could not close file %s: %s\n", progname, fcfile, strerror(errno));
> + goto failure;
> }
> - fclose(fp);
> }
>
> + ret = EXIT_SUCCESS;
> +
> +cleanup:
> + if (fp)
> + fclose(fp);
> + sepol_policy_file_free(out);
> sepol_module_package_free(pkg);
> - exit(0);
> + sepol_policy_file_free(in);
> +
> + return ret;
> +
> +failure:
> + ret = EXIT_FAILURE;
> + goto cleanup;
Same comment as patch 1 about ending with "return ret".
Thanks,
Jim
> }
> --
> 2.40.1
>
