| Message ID | 1441622536-15123-1-git-send-email-imran.zaman@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Monday 2015-09-07 12:42, Imran Zaman wrote: >@@ -579,7 +580,8 @@ static void *load_extension(const char *search_path, const char *af_prefix, > strerror(errno)); > return NULL; > } >- if (dlopen(path, RTLD_NOW) == NULL) { >+ handle = dlopen(path, RTLD_NOW); >+ if (handle == NULL) { > fprintf(stderr, "%s: %s\n", path, dlerror()); > break; > } >@@ -590,6 +592,7 @@ static void *load_extension(const char *search_path, const char *af_prefix, > ptr = xtables_find_match(name, > XTF_DONT_LOAD, NULL); > >+ dlclose(handle); > if (ptr != NULL) > return ptr; > Seriously? If you close the handle, then ptr->init will point to invalid memory and the thing should crash. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Aah. ookey.. so it needs to be moved two lines below.. What about the caller of the function? is it getting closed at any stage?
On Monday 2015-09-07 12:52, Zaman, Imran wrote: >Aah. ookey.. so it needs to be moved two lines below.. At which point it's almost useless, because the program will end if the requested extension cannot be loaded. >What about the caller of the function? is it getting closed at any stage? Implicitly at program exit. It's not pretty valgrind-wise, but implicit resource freeing seems a not-uncommon practice for some programs. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Well! I tend to disagree here.. Any resource should be freed immediately if it is not in use.. function does not know how all the variables on heap be handled outside of the function, if those are not dealt internally unless returned in all cases to the function caller.. Besides, static analysis of the code is reporting resource leak at this code block and it should be fixed for the same reason and will be more clearer for callers of this function. Freeing resource immediately once it is not in use is normal practice and must be followed in all cases as how I see it :) Do you know when is 'ptr' get free'd and at which point in code? sorry i m not very familiar with all iptables codebase, so if you can guide me I can sure come up with proper fix for it.. thanks
On Tuesday 2015-09-08 09:02, Zaman, Imran wrote: >Well! I tend to disagree here.. > >Any resource should be freed immediately if it is not in use.. Yet they are in use until xtables-main.c:78 or so. >Do you know when is 'ptr' get free'd and at which point in code? xtables-main.c:93: exit(!ret) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
You meant xtables.c as I couldnt find xtables-main.c or am I looking at wrong repo/branch
On Tuesday 2015-09-08 11:50, Zaman, Imran wrote: >You meant xtables.c as I couldnt find xtables-main.c or am I looking at wrong repo/branch xtables-standalone.c, iptables-standalone.c and ip6tables-standalone.c they are. >>Well! I tend to disagree here.. >>Any resource should be freed immediately if it is not in use.. > >Yet they are in use until xtables-main.c:78 or so. > >>Do you know when is 'ptr' get free'd and at which point in code? > >xtables-main.c:93: exit(!ret) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 08, 2015 at 10:55:30AM +0200, Jan Engelhardt wrote: > On Tuesday 2015-09-08 09:02, Zaman, Imran wrote: > > >Well! I tend to disagree here.. > > > >Any resource should be freed immediately if it is not in use.. > > Yet they are in use until xtables-main.c:78 or so. This lazy resource approach is sloppy. It's a good practise to release things before leaving the program. By releasing things when we don't need them anymore, we reduce the noise in valgrind and similar tools so it becomes more easily to detect more significant problems in the software. Imran, please fix your patch to avoid the crash that was indicated in first place and resubmit, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/libxtables/xtables.c b/libxtables/xtables.c index 5357d12..e684d27 100644 --- a/libxtables/xtables.c +++ b/libxtables/xtables.c @@ -561,6 +561,7 @@ static void *load_extension(const char *search_path, const char *af_prefix, void *ptr = NULL; struct stat sb; char path[256]; + void *handle = NULL; do { next = strchr(dir, ':'); @@ -579,7 +580,8 @@ static void *load_extension(const char *search_path, const char *af_prefix, strerror(errno)); return NULL; } - if (dlopen(path, RTLD_NOW) == NULL) { + handle = dlopen(path, RTLD_NOW); + if (handle == NULL) { fprintf(stderr, "%s: %s\n", path, dlerror()); break; } @@ -590,6 +592,7 @@ static void *load_extension(const char *search_path, const char *af_prefix, ptr = xtables_find_match(name, XTF_DONT_LOAD, NULL); + dlclose(handle); if (ptr != NULL) return ptr;
dlclose is used to close the handle returned by dlopen. Signed-off-by: Imran Zaman <imran.zaman@intel.com> --- libxtables/xtables.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)