Re: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

Re: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Jan Engelhardt]

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.

[PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Imran Zaman]

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(-)

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;
 
-- 
1.9.1

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Zaman, Imran]

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?
________________________________________
From: Jan Engelhardt [jengelh@inai.de]
Sent: 07 September 2015 13:41
To: Zaman, Imran
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

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.
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Jan Engelhardt]

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.

RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Zaman, Imran]

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

________________________________________
From: Jan Engelhardt [jengelh@inai.de]
Sent: 07 September 2015 16:08
To: Zaman, Imran
Cc: netfilter-devel@vger.kernel.org
Subject: RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

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.
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Jan Engelhardt]

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)

RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Zaman, Imran]

You meant xtables.c as I couldnt find xtables-main.c or am I looking at wrong repo/branch

________________________________________
From: Jan Engelhardt [jengelh@inai.de]
Sent: 08 September 2015 11:55
To: Zaman, Imran
Cc: netfilter-devel@vger.kernel.org
Subject: RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

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)
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Jan Engelhardt]

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)

Re: [PATCH v1 1/1] fix: resource leakage when loading library using dlopen

[Pablo Neira Ayuso]

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.