On Fri, May 12, 2023 at 7:28 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.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> .../semodule_expand/semodule_expand.c | 91 +++++++++++--------
> 1 file changed, 55 insertions(+), 36 deletions(-)
>
> diff --git a/semodule-utils/semodule_expand/semodule_expand.c b/semodule-utils/semodule_expand/semodule_expand.c
> index 895cdd78..8d9feb05 100644
> --- a/semodule-utils/semodule_expand/semodule_expand.c
> +++ b/semodule-utils/semodule_expand/semodule_expand.c
> @@ -21,30 +21,24 @@
> #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)
> {
> - printf("usage: %s [-V -a -c [version]] basemodpkg outputfile\n",
> + printf("usage: %s [-V -a -c [version] -v] basemodpkg outputfile\n",
> program_name);
> exit(1);
If you are going to use EXIT_FAILURE, then all the uses of "exit(1)"
should be removed.
> }
>
> 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;
> + 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:Vva")) != EOF) {
> switch (ch) {
> @@ -55,13 +49,15 @@ int main(int argc, char **argv)
> verbose = 1;
> break;
> 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);
> }
> if (n < sepol_policy_kern_vers_min()
> || n > sepol_policy_kern_vers_max()) {
> @@ -71,7 +67,6 @@ int main(int argc, char **argv)
> sepol_policy_kern_vers_min(),
> sepol_policy_kern_vers_max());
> usage(argv[0]);
> - exit(1);
> }
> policyvers = n;
> break;
> @@ -96,7 +91,7 @@ int main(int argc, char **argv)
> }
>
> /* 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]);
> @@ -107,69 +102,74 @@ int main(int argc, char **argv)
> 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");
> 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");
> 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 +178,31 @@ 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;
> +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 ret;
>
> - return 0;
> +failure:
> + ret = EXIT_FAILURE;
> + goto cleanup;
> }
I would prefer to have the return at the end.
Something like this:
ret = EXIT_SUCCESS;
goto cleanup;
failure:
ret = EXIT_FAILURE;
cleanup:
...
return ret;
}
Thanks,
Jim
> --
> 2.40.1
>
