[ndctl PATCH] ndctl, monitor: fix a resource leak in parse_monitor_event

[ndctl PATCH] ndctl, monitor: fix a resource leak in parse_monitor_event

[Vishal Verma]

Static analysis reports we leak dimm_event in the above function.
Fix it by adding an 'out' label for all exit paths, and refactoring out
the 'all events' cases.

Fixes: fdf6b6844ccf ("ndctl, monitor: add a new command - monitor")
Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/monitor.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index dbad7aa..b97d1ea 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -410,22 +410,35 @@ static int monitor_event(struct ndctl_ctx *ctx,
 	return rc;
 }
 
+static void monitor_enable_all_events(struct monitor *_monitor)
+{
+	_monitor->event_flags = ND_EVENT_SPARES_REMAINING
+			| ND_EVENT_MEDIA_TEMPERATURE
+			| ND_EVENT_CTRL_TEMPERATURE
+			| ND_EVENT_HEALTH_STATE
+			| ND_EVENT_UNCLEAN_SHUTDOWN;
+}
+
 static int parse_monitor_event(struct monitor *_monitor, struct ndctl_ctx *ctx)
 {
 	char *dimm_event, *save;
 	const char *event;
+	int rc = 0;
+
+	if (!_monitor->dimm_event) {
+		monitor_enable_all_events(_monitor);
+		return 0;;
+	}
 
-	if (!_monitor->dimm_event)
-		goto dimm_event_all;
 	dimm_event = strdup(_monitor->dimm_event);
 	if (!dimm_event)
-		return 1;
+		return -ENOMEM;
 
 	for (event = strtok_r(dimm_event, " ", &save); event;
 			event = strtok_r(NULL, " ", &save)) {
 		if (strcmp(event, "all") == 0) {
-			free(dimm_event);
-			goto dimm_event_all;
+			monitor_enable_all_events(_monitor);
+			goto out;
 		}
 		if (strcmp(event, "dimm-spares-remaining") == 0)
 			_monitor->event_flags |= ND_EVENT_SPARES_REMAINING;
@@ -439,20 +452,14 @@ static int parse_monitor_event(struct monitor *_monitor, struct ndctl_ctx *ctx)
 			_monitor->event_flags |= ND_EVENT_UNCLEAN_SHUTDOWN;
 		else {
 			err(ctx, "no dimm-event named %s\n", event);
-			return 1;
+			rc = -EINVAL;
+			goto out;
 		}
 	}
 
+out:
 	free(dimm_event);
-	return 0;
-
-dimm_event_all:
-	_monitor->event_flags = ND_EVENT_SPARES_REMAINING
-			| ND_EVENT_MEDIA_TEMPERATURE
-			| ND_EVENT_CTRL_TEMPERATURE
-			| ND_EVENT_HEALTH_STATE
-			| ND_EVENT_UNCLEAN_SHUTDOWN;
-	return 0;
+	return rc;
 }
 
 static void parse_config(const char **arg, char *key, char *val, char *ident)
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndctl PATCH] ndctl, monitor: fix a resource leak in parse_monitor_event

[Qi, Fuli]

> -----Original Message-----
> From: Vishal Verma [mailto:vishal.l.verma@intel.com]
> Sent: Wednesday, July 25, 2018 9:09 AM
> To: linux-nvdimm@lists.01.org
> Cc: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; Vishal Verma
> <vishal.l.verma@intel.com>
> Subject: [ndctl PATCH] ndctl, monitor: fix a resource leak in parse_monitor_event
> 
> Static analysis reports we leak dimm_event in the above function.
> Fix it by adding an 'out' label for all exit paths, and refactoring out the 'all
> events' cases.
> 
> Fixes: fdf6b6844ccf ("ndctl, monitor: add a new command - monitor")
> Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/monitor.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index dbad7aa..b97d1ea 100644
> --- a/ndctl/monitor.c
> +++ b/ndctl/monitor.c
> @@ -410,22 +410,35 @@ static int monitor_event(struct ndctl_ctx *ctx,
>  	return rc;
>  }
> 
> +static void monitor_enable_all_events(struct monitor *_monitor) {
> +	_monitor->event_flags = ND_EVENT_SPARES_REMAINING
> +			| ND_EVENT_MEDIA_TEMPERATURE
> +			| ND_EVENT_CTRL_TEMPERATURE
> +			| ND_EVENT_HEALTH_STATE
> +			| ND_EVENT_UNCLEAN_SHUTDOWN;
> +}
> +
>  static int parse_monitor_event(struct monitor *_monitor, struct ndctl_ctx *ctx)
> {
>  	char *dimm_event, *save;
>  	const char *event;
> +	int rc = 0;
> +
> +	if (!_monitor->dimm_event) {
> +		monitor_enable_all_events(_monitor);
> +		return 0;;
> +	}
> 
> -	if (!_monitor->dimm_event)
> -		goto dimm_event_all;
>  	dimm_event = strdup(_monitor->dimm_event);
>  	if (!dimm_event)
> -		return 1;
> +		return -ENOMEM;
> 
>  	for (event = strtok_r(dimm_event, " ", &save); event;
>  			event = strtok_r(NULL, " ", &save)) {
>  		if (strcmp(event, "all") == 0) {
> -			free(dimm_event);
> -			goto dimm_event_all;
> +			monitor_enable_all_events(_monitor);
> +			goto out;
>  		}
>  		if (strcmp(event, "dimm-spares-remaining") == 0)
>  			_monitor->event_flags |= ND_EVENT_SPARES_REMAINING; @@
> -439,20 +452,14 @@ static int parse_monitor_event(struct monitor *_monitor, struct
> ndctl_ctx *ctx)
>  			_monitor->event_flags |= ND_EVENT_UNCLEAN_SHUTDOWN;
>  		else {
>  			err(ctx, "no dimm-event named %s\n", event);
> -			return 1;
> +			rc = -EINVAL;
> +			goto out;
>  		}
>  	}
> 
> +out:
>  	free(dimm_event);
> -	return 0;
> -
> -dimm_event_all:
> -	_monitor->event_flags = ND_EVENT_SPARES_REMAINING
> -			| ND_EVENT_MEDIA_TEMPERATURE
> -			| ND_EVENT_CTRL_TEMPERATURE
> -			| ND_EVENT_HEALTH_STATE
> -			| ND_EVENT_UNCLEAN_SHUTDOWN;
> -	return 0;
> +	return rc;
>  }
> 

Looks good to me.
Please feel free to add: Reviewed-by: QI Fuli <qi.fuli@jp.fujitsu.com>

Thanks,
QI

>  static void parse_config(const char **arg, char *key, char *val, char *ident)
> --
> 2.14.4
> 
> 


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm