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

[Petr Lautrbach <plautrba@redhat.com>]

Nicolas Iooss <nicolas.iooss@m4x.org> writes:

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


Diff between scans before and after shows that it fixes at least 
the two
following defects:

Error: USE_AFTER_FREE (CWE-672):
libsemanage-2.8/src/direct_api.c:2142: freed_arg: "fclose" frees 
"fp".
libsemanage-2.8/src/direct_api.c:2180: use_closed_file: Calling 
"fclose" uses file handle "fp" after closing it.
# 2178|   	free(modinfo);
# 2179|   
# 2180|-> 	if (fp != NULL) fclose(fp);
# 2181|   	return status;
# 2182|   }

Error: USE_AFTER_FREE (CWE-672):
libsemanage-2.8/src/direct_api.c:2345: freed_arg: "fclose" frees 
"fp".
libsemanage-2.8/src/direct_api.c:2398: use_closed_file: Calling 
"fclose" uses file handle "fp" after closing it.
# 2396|   	}
# 2397|   
# 2398|-> 	if (fp != NULL) fclose(fp);
# 2399|   	return status;
# 2400|   }


But it was supposed to fix more defects and reading it again it 
would be
better to assign NULL to fp on line 2349 instead of moving the 
whole
code block to cleanup.

I'll resend new set of patches with better commit messages.

Thanks!

> Otherwise the content of the patch looks good to me.
>
> Nicolas