| Message ID | 20170407204431.8572-2-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, 2017-04-07 at 22:44 +0200, Nicolas Iooss wrote: > semanage_module_info_destroy() always returns 0. Nevertheless > semanage_direct_list_all() uses its return value in a surprising way: > > cleanup: > if (priorities != NULL) { > /* ... */ > free(priorities); > } > /* ... */ > ret = semanage_module_info_destroy(sh, modinfo_tmp); > if (ret != 0) { > status = -1; > goto cleanup; > } > > The last "goto cleanup;" leads clang's static analyzer to believe a > double free is possible. Even though this is a false positive, the > body of condition "if (ret != 0)" contains dead code. Remove it. I'm merging this one but we should likely follow up with a patch to change semodule_module_info_destroy() to explicitly return void. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > --- > libsemanage/src/direct_api.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > > diff --git a/libsemanage/src/direct_api.c > b/libsemanage/src/direct_api.c > index 568732355f54..1d53c0d64e0c 100644 > --- a/libsemanage/src/direct_api.c > +++ b/libsemanage/src/direct_api.c > @@ -2499,11 +2499,7 @@ static int > semanage_direct_list_all(semanage_handle_t *sh, > goto cleanup; > } > > - ret = semanage_module_info_destroy(sh, > modinfo_tmp); > - if (ret != 0) { > - status = -1; > - goto cleanup; > - } > + semanage_module_info_destroy(sh, > modinfo_tmp); > free(modinfo_tmp); > modinfo_tmp = NULL; > > @@ -2528,11 +2524,7 @@ cleanup: > free(modules); > } > > - ret = semanage_module_info_destroy(sh, modinfo_tmp); > - if (ret != 0) { > - status = -1; > - goto cleanup; > - } > + semanage_module_info_destroy(sh, modinfo_tmp); > free(modinfo_tmp); > modinfo_tmp = NULL; >
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index 568732355f54..1d53c0d64e0c 100644 --- a/libsemanage/src/direct_api.c +++ b/libsemanage/src/direct_api.c @@ -2499,11 +2499,7 @@ static int semanage_direct_list_all(semanage_handle_t *sh, goto cleanup; } - ret = semanage_module_info_destroy(sh, modinfo_tmp); - if (ret != 0) { - status = -1; - goto cleanup; - } + semanage_module_info_destroy(sh, modinfo_tmp); free(modinfo_tmp); modinfo_tmp = NULL; @@ -2528,11 +2524,7 @@ cleanup: free(modules); } - ret = semanage_module_info_destroy(sh, modinfo_tmp); - if (ret != 0) { - status = -1; - goto cleanup; - } + semanage_module_info_destroy(sh, modinfo_tmp); free(modinfo_tmp); modinfo_tmp = NULL;
semanage_module_info_destroy() always returns 0. Nevertheless semanage_direct_list_all() uses its return value in a surprising way: cleanup: if (priorities != NULL) { /* ... */ free(priorities); } /* ... */ ret = semanage_module_info_destroy(sh, modinfo_tmp); if (ret != 0) { status = -1; goto cleanup; } The last "goto cleanup;" leads clang's static analyzer to believe a double free is possible. Even though this is a false positive, the body of condition "if (ret != 0)" contains dead code. Remove it. Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsemanage/src/direct_api.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)