[PATCH] tc: Fix descriptor leak in get_qdisc_kind()

[PATCH] tc: Fix descriptor leak in get_qdisc_kind()

[Maks Mishin]

Add closure of fd `dlh` and fix incorrect comparison of `dlh` with NULL.

Found by RASU JSC

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 tc/tc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tc/tc.c b/tc/tc.c
index 575157a8..162700b2 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -112,7 +112,7 @@ struct qdisc_util *get_qdisc_kind(const char *str)
 
 	snprintf(buf, sizeof(buf), "%s/q_%s.so", get_tc_lib(), str);
 	dlh = dlopen(buf, RTLD_LAZY);
-	if (!dlh) {
+	if (dlh != NULL) {
 		/* look in current binary, only open once */
 		dlh = BODY;
 		if (dlh == NULL) {
@@ -124,6 +124,9 @@ struct qdisc_util *get_qdisc_kind(const char *str)
 
 	snprintf(buf, sizeof(buf), "%s_qdisc_util", str);
 	q = dlsym(dlh, buf);
+	if (dlh != NULL)
+		dlclose(dlh);
+
 	if (q == NULL)
 		goto noexist;
 
-- 
2.30.2

Re: [PATCH] tc: Fix descriptor leak in get_qdisc_kind()

[Donald Hunter]

Maks Mishin <maks.mishinfz@gmail.com> writes:

> Add closure of fd `dlh` and fix incorrect comparison of `dlh` with NULL.
>
> Found by RASU JSC

What is this tool? It seems to be giving you bad advice.

> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> ---
>  tc/tc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tc/tc.c b/tc/tc.c
> index 575157a8..162700b2 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -112,7 +112,7 @@ struct qdisc_util *get_qdisc_kind(const char *str)
>  
>  	snprintf(buf, sizeof(buf), "%s/q_%s.so", get_tc_lib(), str);
>  	dlh = dlopen(buf, RTLD_LAZY);
> -	if (!dlh) {
> +	if (dlh != NULL) {

if (!dlh) seems to be the preferred style in the iproute2 codebase, with
2000+ occurrences.

>  		/* look in current binary, only open once */
>  		dlh = BODY;
>  		if (dlh == NULL) {
> @@ -124,6 +124,9 @@ struct qdisc_util *get_qdisc_kind(const char *str)
>  
>  	snprintf(buf, sizeof(buf), "%s_qdisc_util", str);
>  	q = dlsym(dlh, buf);
> +	if (dlh != NULL)
> +		dlclose(dlh);

Incorrect placement of dlclose() before sym q gets dereferenced.

> +
>  	if (q == NULL)
>  		goto noexist;

Re: [PATCH] tc: Fix descriptor leak in get_qdisc_kind()

[Stephen Hemminger]

On Thu,  8 Feb 2024 00:00:06 +0300
Maks Mishin <maks.mishinfz@gmail.com> wrote:

> Add closure of fd `dlh` and fix incorrect comparison of `dlh` with NULL.
> 
> Found by RASU JSC
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> ---
>  tc/tc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tc/tc.c b/tc/tc.c
> index 575157a8..162700b2 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -112,7 +112,7 @@ struct qdisc_util *get_qdisc_kind(const char *str)
>  
>  	snprintf(buf, sizeof(buf), "%s/q_%s.so", get_tc_lib(), str);
>  	dlh = dlopen(buf, RTLD_LAZY);
> -	if (!dlh) {
> +	if (dlh != NULL) {
>  		/* look in current binary, only open once */
>  		dlh = BODY;
>  		if (dlh == NULL) {
> @@ -124,6 +124,9 @@ struct qdisc_util *get_qdisc_kind(const char *str)
>  
>  	snprintf(buf, sizeof(buf), "%s_qdisc_util", str);
>  	q = dlsym(dlh, buf);
> +	if (dlh != NULL)
> +		dlclose(dlh);
> +
>  	if (q == NULL)
>  		goto noexist;
>  

This does make sense, but the patch method is messy.
Please don't modify code outside scope of the fix being addressed.
And since the check for dlh being NULL is already done above,
the conditional there is irrelevant.

Also, this change will break the code. If you read carefully
in the comments, there is a hint.  The idea is that if the
qdisc is not in the external file, then it does an internal
dlopen() and it wants to keep it open in the variable BODY.

Your change would close dlh which since it same as BODY would
close the internal handle.  That would cause next call to get_qdisc_kind()
to look at an invalid handle (BODY).

I appreciate your efforts, but you need to read deeper.
This is 30 year old code and the leak may not be important
or be intentional.