| Message ID | 20190131132226.19030-3-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan | expand |
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
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
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; }
Signed-off-by: Petr Lautrbach <plautrba@redhat.com> --- libsemanage/src/direct_api.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)