Re: [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE defects reported by coverity scan

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libsemanage/src/direct_api.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index c58961be..8e4d116d 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -1028,7 +1028,7 @@ static int semanage_direct_write_langext(semanage_handle_t *sh,
>
>         fp = NULL;
>
> -       ret = 0;
> +       return 0;
>
>  cleanup:
>         if (fp != NULL) fclose(fp);
> @@ -2177,7 +2177,6 @@ cleanup:
>         semanage_module_info_destroy(sh, modinfo);
>         free(modinfo);
>
> -       if (fp != NULL) fclose(fp);
>         return status;
>  }
>
> @@ -2342,16 +2341,6 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>         free(tmp);
>         tmp = NULL;
>
> -       if (fclose(fp) != 0) {
> -               ERR(sh,
> -                   "Unable to close %s module lang ext file.",
> -                   (*modinfo)->name);
> -               status = -1;
> -               goto cleanup;
> -       }
> -
> -       fp = NULL;
> -
>         /* lookup enabled/disabled status */
>         ret = semanage_module_get_path(sh,
>                                        *modinfo,
> @@ -2395,7 +2384,13 @@ cleanup:
>                 free(modinfos);
>         }
>
> -       if (fp != NULL) fclose(fp);
> +       if (fp != NULL && fclose(fp) != 0) {
> +               ERR(sh,
> +                   "Unable to close %s module lang ext file.",
> +                   (*modinfo)->name);
> +               status = -1;
> +       }
> +
>         return status;
>  }
>
> --
> 2.20.1

After reading this patch, I am wondering what the USE_AFTER_FREE
defects were about. Were there real use-after-fclose bugs that are
fixed by this patch? Or is this patch more about silencing Coverity's
false positives due to difficulties in parsing constructions such as
"fclose(fp);fp = NULL; if(f != NULL)fclose(fp);"? It would be useful
if the patch description contains answers to these questions.

Otherwise the content of the patch looks good to me.

Nicolas