[ofa-general] Re: [PATCH] opensm/osm_subnet.c Fix memory leak for QOS string parameters.

[Sasha Khapyorsky]

Hi Eli,

On 13:54 Sun 21 Dec     , Eli Dorfman (Voltaire) wrote:
> Fix memory leak for QOS string parameters.
> 
>  Signed-off-by: Slava Strebkov <slavas at amirm.voltaire.com>
> 
> ---
>  opensm/opensm/osm_subnet.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> index 122d4dd..f8b29f8 100644
> --- a/opensm/opensm/osm_subnet.c
> +++ b/opensm/opensm/osm_subnet.c
> @@ -331,6 +331,21 @@ static void subn_init_qos_options(IN osm_qos_options_t * opt)
>  	opt->sl2vl = NULL;
>  }
>  
> +static void subn_free_qos_options(IN osm_qos_options_t * opt)
> +{
> +	if ((opt->vlarb_high) && (opt->vlarb_high != OSM_DEFAULT_QOS_VLARB_HIGH)) {
> +		free(opt->vlarb_high);
> +	}
> +
> +	if ((opt->vlarb_low) && (opt->vlarb_low != OSM_DEFAULT_QOS_VLARB_LOW)) {
> +		free(opt->vlarb_low);
> +	}
> +
> +	if ((opt->sl2vl) && (opt->sl2vl != OSM_DEFAULT_QOS_SL2VL)) {
> +		free(opt->sl2vl);
> +	}
> +}

With gcc-4.3.2 using '-Wall' flag I get warning here:

"comparison with string literal results in unspecified behavior"

It is actually true since used OSM_DEFAULT_QOS_* macros are defined as
strings - it is something equal to:

	char *p = "123456";

	....

	if (p != "123456")
		free(p1);

Gcc is smart enough and uses the same string constant "123456" in both
cases (so your patch actually works), but don't think that it is
guaranteed in C language.

So I think to rework this part - we can always allocate string qos config
parameters (just similar to other config), and free it when it is not
NULL. Something like this:


diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
index a6db304..94b6332 100644
--- a/opensm/opensm/osm_subnet.c
+++ b/opensm/opensm/osm_subnet.c
@@ -333,13 +333,13 @@ static void subn_init_qos_options(IN osm_qos_options_t * opt)
 
 static void subn_free_qos_options(IN osm_qos_options_t * opt)
 {
-	if (opt->vlarb_high && opt->vlarb_high != OSM_DEFAULT_QOS_VLARB_HIGH)
+	if (opt->vlarb_high)
 		free(opt->vlarb_high);
 
-	if (opt->vlarb_low && opt->vlarb_low != OSM_DEFAULT_QOS_VLARB_LOW)
+	if (opt->vlarb_low)
 		free(opt->vlarb_low);
 
-	if (opt->sl2vl && opt->sl2vl != OSM_DEFAULT_QOS_SL2VL)
+	if (opt->sl2vl)
 		free(opt->sl2vl);
 }
 
@@ -803,7 +803,7 @@ static void subn_verify_vlarb(char **vlarb, const char *prefix,
 	if (*vlarb == NULL) {
 		log_report(" Invalid Cached Option: %s_vlarb_%s: "
 		"Using Default\n", prefix, suffix);
-		*vlarb = dflt;
+		*vlarb = strdup(dflt);
 		return;
 	}
 
@@ -872,7 +872,7 @@ static void subn_verify_sl2vl(char **sl2vl, const char *prefix, char *dflt)
 	if (*sl2vl == NULL) {
 		log_report(" Invalid Cached Option: %s_sl2vl: Using Default\n",
 			   prefix);
-		*sl2vl = dflt;
+		*sl2vl = strdup(dflt);
 		return;
 	}


Thoughts?

Sasha