Nicolas Iooss <nicolas.iooss@xxxxxxx> writes:
On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach
<plautrba@xxxxxxxxxx> wrote:
Signed-off-by: Petr Lautrbach <plautrba@xxxxxxxxxx>
---
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
