[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 <sla...@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
_______________________________________________
general mailing list
general@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general