NVDIMM Device and Persistent Memory development
 help / color / Atom feed
* [ndctl PATCH v2 0/2] fix two issues reported by Coverity
@ 2021-07-06  4:31 Zhiqiang Liu
  2021-07-06  4:32 ` [ndctl PATCH v2 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
  2021-07-06  4:33 ` [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace() Zhiqiang Liu
  0 siblings, 2 replies; 5+ messages in thread
From: Zhiqiang Liu @ 2021-07-06  4:31 UTC (permalink / raw)
  To: vishal.l.verma
  Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26, Alison Schofield


Recently, we use Coverity to analysis the ndctl package, again.
Two issues should be resolved to make Coverity happy.

Zhiqiang Liu (2):
  libndctl: check return value of ndctl_pfn_get_namespace
  namespace: fix potentail fd leak problem in do_xaction_namespace()

 ndctl/namespace.c | 35 +++++++++++++++++++++++------------
 test/libndctl.c   |  4 ++--
 util/json.c       |  2 ++
 3 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.23.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [ndctl PATCH v2 1/2] libndctl: check return value of ndctl_pfn_get_namespace
  2021-07-06  4:31 [ndctl PATCH v2 0/2] fix two issues reported by Coverity Zhiqiang Liu
@ 2021-07-06  4:32 ` Zhiqiang Liu
  2021-07-06  4:33 ` [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace() Zhiqiang Liu
  1 sibling, 0 replies; 5+ messages in thread
From: Zhiqiang Liu @ 2021-07-06  4:32 UTC (permalink / raw)
  To: vishal.l.verma
  Cc: nvdimm, linfeilong, lixiaokeng, Alison Schofield, liuzhiqiang26


Recently, we use Coverity to analysis the ndctl package,
one kind of NULL_RETURNS issue is reported as follows,
pfn_clear_badblocks():
	CID 11690495: (NULL_RETURNS)
    1429. dereference: Dereferencing a pointer that might be "NULL" "ndns" when calling "ndctl_namespace_disable_safe".
dax_clear_badblocks():
	CID 11690504: (NULL_RETURNS)
    1405. dereference: Dereferencing a pointer that might be "NULL" "ndns" when calling "ndctl_namespace_disable_safe".
util_pfn_badblocks_to_json():
	CID 11690524: (NULL_RETURNS)
    812. dereference: Dereferencing a pointer that might be "NULL" "ndns" when calling "util_namespace_badblocks_to_json".

ndctl_pfn_get_namespace() may return NULL, so callers
should check return value of it. Otherwise, it may
cause access NULL pointer problem.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
v1->v2: add coverity report info as suggested by Alison

 ndctl/namespace.c | 18 ++++++++++++++----
 test/libndctl.c   |  4 ++--
 util/json.c       |  2 ++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 0c8df9f..21089d7 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1417,11 +1417,16 @@ static int nstype_clear_badblocks(struct ndctl_namespace *ndns,

 static int dax_clear_badblocks(struct ndctl_dax *dax)
 {
-	struct ndctl_namespace *ndns = ndctl_dax_get_namespace(dax);
-	const char *devname = ndctl_dax_get_devname(dax);
+	struct ndctl_namespace *ndns;
+	const char *devname;
 	unsigned long long begin, size;
 	int rc;

+	ndns = ndctl_dax_get_namespace(dax);
+	if (!ndns)
+		return -ENXIO;
+
+	devname = ndctl_dax_get_devname(dax);
 	begin = ndctl_dax_get_resource(dax);
 	if (begin == ULLONG_MAX)
 		return -ENXIO;
@@ -1441,11 +1446,16 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)

 static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
 {
-	struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
-	const char *devname = ndctl_pfn_get_devname(pfn);
+	struct ndctl_namespace *ndns;
+	const char *devname;
 	unsigned long long begin, size;
 	int rc;

+	ndns = ndctl_pfn_get_namespace(pfn);
+	if (!ndns)
+		return -ENXIO;
+
+	devname = ndctl_pfn_get_devname(pfn);
 	begin = ndctl_pfn_get_resource(pfn);
 	if (begin == ULLONG_MAX)
 		return -ENXIO;
diff --git a/test/libndctl.c b/test/libndctl.c
index 24d72b3..05e5ff2 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -1275,7 +1275,7 @@ static int check_pfn_autodetect(struct ndctl_bus *bus,
 		if (!ndctl_pfn_is_enabled(pfn))
 			continue;
 		pfn_ndns = ndctl_pfn_get_namespace(pfn);
-		if (strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
+		if (!pfn_ndns || strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
 			continue;
 		fprintf(stderr, "%s: pfn_ndns: %p ndns: %p\n", __func__,
 				pfn_ndns, ndns);
@@ -1372,7 +1372,7 @@ static int check_dax_autodetect(struct ndctl_bus *bus,
 		if (!ndctl_dax_is_enabled(dax))
 			continue;
 		dax_ndns = ndctl_dax_get_namespace(dax);
-		if (strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
+		if (!dax_ndns || strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
 			continue;
 		fprintf(stderr, "%s: dax_ndns: %p ndns: %p\n", __func__,
 				dax_ndns, ndns);
diff --git a/util/json.c b/util/json.c
index ca0167b..249f021 100644
--- a/util/json.c
+++ b/util/json.c
@@ -1002,6 +1002,8 @@ static struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn,
 	pfn_begin = ndctl_pfn_get_resource(pfn);
 	if (pfn_begin == ULLONG_MAX) {
 		struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
+		if (!ndns)
+			return NULL;

 		return util_namespace_badblocks_to_json(ndns, bb_count, flags);
 	}
-- 
2.23.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace()
  2021-07-06  4:31 [ndctl PATCH v2 0/2] fix two issues reported by Coverity Zhiqiang Liu
  2021-07-06  4:32 ` [ndctl PATCH v2 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
@ 2021-07-06  4:33 ` Zhiqiang Liu
  2021-07-15  0:58   ` Zhiqiang Liu
  2021-07-31  8:31   ` Zhiqiang Liu
  1 sibling, 2 replies; 5+ messages in thread
From: Zhiqiang Liu @ 2021-07-06  4:33 UTC (permalink / raw)
  To: vishal.l.verma
  Cc: nvdimm, linfeilong, lixiaokeng, Alison Schofield, liuzhiqiang26


Recently, we use Coverity to analysis the ndctl package,
one issue in do_xaction_namespace() is reported as follows,
	CID 11690564: (RESOURCE_LEAK)
	2058. leaked_storage: Variable "ri_ctx" going out of scope
	      leaks the storage "ri_ctx.f_out" points to.

In do_xaction_namespace(), ri_ctx.f_out should be closed after
being opened. This prevents a potential file descriptor leak
in do_xaction_namespace().

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
v1->v2: add coverity report info as suggested by Alison

 ndctl/namespace.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 21089d7..55364ac 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2141,7 +2141,7 @@ static int do_xaction_namespace(const char *namespace,
 				util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
 			if (rc >= 0)
 				(*processed)++;
-			return rc;
+			goto out;
 		}
 	}

@@ -2152,11 +2152,11 @@ static int do_xaction_namespace(const char *namespace,
 		rc = file_write_infoblock(param.outfile);
 		if (rc >= 0)
 			(*processed)++;
-		return rc;
+		goto out;
 	}

 	if (!namespace && action != ACTION_CREATE)
-		return rc;
+		goto out;

 	if (verbose)
 		ndctl_set_log_priority(ctx, LOG_DEBUG);
@@ -2212,7 +2212,7 @@ static int do_xaction_namespace(const char *namespace,
 						saved_rc = rc;
 						continue;
 				}
-				return rc;
+				goto out;
 			}
 			ndctl_namespace_foreach_safe(region, ndns, _n) {
 				ndns_name = ndctl_namespace_get_devname(ndns);
@@ -2259,7 +2259,7 @@ static int do_xaction_namespace(const char *namespace,
 					rc = namespace_reconfig(region, ndns);
 					if (rc == 0)
 						*processed = 1;
-					return rc;
+					goto out;
 				case ACTION_READ_INFOBLOCK:
 					rc = namespace_rw_infoblock(ndns, &ri_ctx, READ);
 					if (rc == 0)
@@ -2281,9 +2281,6 @@ static int do_xaction_namespace(const char *namespace,
 	if (ri_ctx.jblocks)
 		util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);

-	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
-		fclose(ri_ctx.f_out);
-
 	if (action == ACTION_CREATE && rc == -EAGAIN) {
 		/*
 		 * Namespace creation searched through all candidate
@@ -2301,6 +2298,10 @@ static int do_xaction_namespace(const char *namespace,
 	if (saved_rc)
 		rc = saved_rc;

+out:
+	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
+		fclose(ri_ctx.f_out);
+
 	return rc;
 }

-- 
2.23.0




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace()
  2021-07-06  4:33 ` [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace() Zhiqiang Liu
@ 2021-07-15  0:58   ` Zhiqiang Liu
  2021-07-31  8:31   ` Zhiqiang Liu
  1 sibling, 0 replies; 5+ messages in thread
From: Zhiqiang Liu @ 2021-07-15  0:58 UTC (permalink / raw)
  To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, Alison Schofield

friendly ping...

On 2021/7/6 12:33, Zhiqiang Liu wrote:
> Recently, we use Coverity to analysis the ndctl package,
> one issue in do_xaction_namespace() is reported as follows,
> 	CID 11690564: (RESOURCE_LEAK)
> 	2058. leaked_storage: Variable "ri_ctx" going out of scope
> 	      leaks the storage "ri_ctx.f_out" points to.
>
> In do_xaction_namespace(), ri_ctx.f_out should be closed after
> being opened. This prevents a potential file descriptor leak
> in do_xaction_namespace().
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> v1->v2: add coverity report info as suggested by Alison
>
>  ndctl/namespace.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index 21089d7..55364ac 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -2141,7 +2141,7 @@ static int do_xaction_namespace(const char *namespace,
>  				util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>  			if (rc >= 0)
>  				(*processed)++;
> -			return rc;
> +			goto out;
>  		}
>  	}
>
> @@ -2152,11 +2152,11 @@ static int do_xaction_namespace(const char *namespace,
>  		rc = file_write_infoblock(param.outfile);
>  		if (rc >= 0)
>  			(*processed)++;
> -		return rc;
> +		goto out;
>  	}
>
>  	if (!namespace && action != ACTION_CREATE)
> -		return rc;
> +		goto out;
>
>  	if (verbose)
>  		ndctl_set_log_priority(ctx, LOG_DEBUG);
> @@ -2212,7 +2212,7 @@ static int do_xaction_namespace(const char *namespace,
>  						saved_rc = rc;
>  						continue;
>  				}
> -				return rc;
> +				goto out;
>  			}
>  			ndctl_namespace_foreach_safe(region, ndns, _n) {
>  				ndns_name = ndctl_namespace_get_devname(ndns);
> @@ -2259,7 +2259,7 @@ static int do_xaction_namespace(const char *namespace,
>  					rc = namespace_reconfig(region, ndns);
>  					if (rc == 0)
>  						*processed = 1;
> -					return rc;
> +					goto out;
>  				case ACTION_READ_INFOBLOCK:
>  					rc = namespace_rw_infoblock(ndns, &ri_ctx, READ);
>  					if (rc == 0)
> @@ -2281,9 +2281,6 @@ static int do_xaction_namespace(const char *namespace,
>  	if (ri_ctx.jblocks)
>  		util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>
> -	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
> -		fclose(ri_ctx.f_out);
> -
>  	if (action == ACTION_CREATE && rc == -EAGAIN) {
>  		/*
>  		 * Namespace creation searched through all candidate
> @@ -2301,6 +2298,10 @@ static int do_xaction_namespace(const char *namespace,
>  	if (saved_rc)
>  		rc = saved_rc;
>
> +out:
> +	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
> +		fclose(ri_ctx.f_out);
> +
>  	return rc;
>  }
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace()
  2021-07-06  4:33 ` [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace() Zhiqiang Liu
  2021-07-15  0:58   ` Zhiqiang Liu
@ 2021-07-31  8:31   ` Zhiqiang Liu
  1 sibling, 0 replies; 5+ messages in thread
From: Zhiqiang Liu @ 2021-07-31  8:31 UTC (permalink / raw)
  To: vishal.l.verma
  Cc: nvdimm, linfeilong, lixiaokeng, Alison Schofield, liuzhiqiang26

friendly ping...

On 2021/7/6 12:33, Zhiqiang Liu wrote:
> Recently, we use Coverity to analysis the ndctl package,
> one issue in do_xaction_namespace() is reported as follows,
> 	CID 11690564: (RESOURCE_LEAK)
> 	2058. leaked_storage: Variable "ri_ctx" going out of scope
> 	      leaks the storage "ri_ctx.f_out" points to.
>
> In do_xaction_namespace(), ri_ctx.f_out should be closed after
> being opened. This prevents a potential file descriptor leak
> in do_xaction_namespace().
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> v1->v2: add coverity report info as suggested by Alison
>
>  ndctl/namespace.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index 21089d7..55364ac 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -2141,7 +2141,7 @@ static int do_xaction_namespace(const char *namespace,
>  				util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>  			if (rc >= 0)
>  				(*processed)++;
> -			return rc;
> +			goto out;
>  		}
>  	}
>
> @@ -2152,11 +2152,11 @@ static int do_xaction_namespace(const char *namespace,
>  		rc = file_write_infoblock(param.outfile);
>  		if (rc >= 0)
>  			(*processed)++;
> -		return rc;
> +		goto out;
>  	}
>
>  	if (!namespace && action != ACTION_CREATE)
> -		return rc;
> +		goto out;
>
>  	if (verbose)
>  		ndctl_set_log_priority(ctx, LOG_DEBUG);
> @@ -2212,7 +2212,7 @@ static int do_xaction_namespace(const char *namespace,
>  						saved_rc = rc;
>  						continue;
>  				}
> -				return rc;
> +				goto out;
>  			}
>  			ndctl_namespace_foreach_safe(region, ndns, _n) {
>  				ndns_name = ndctl_namespace_get_devname(ndns);
> @@ -2259,7 +2259,7 @@ static int do_xaction_namespace(const char *namespace,
>  					rc = namespace_reconfig(region, ndns);
>  					if (rc == 0)
>  						*processed = 1;
> -					return rc;
> +					goto out;
>  				case ACTION_READ_INFOBLOCK:
>  					rc = namespace_rw_infoblock(ndns, &ri_ctx, READ);
>  					if (rc == 0)
> @@ -2281,9 +2281,6 @@ static int do_xaction_namespace(const char *namespace,
>  	if (ri_ctx.jblocks)
>  		util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>
> -	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
> -		fclose(ri_ctx.f_out);
> -
>  	if (action == ACTION_CREATE && rc == -EAGAIN) {
>  		/*
>  		 * Namespace creation searched through all candidate
> @@ -2301,6 +2298,10 @@ static int do_xaction_namespace(const char *namespace,
>  	if (saved_rc)
>  		rc = saved_rc;
>
> +out:
> +	if (ri_ctx.f_out && ri_ctx.f_out != stdout)
> +		fclose(ri_ctx.f_out);
> +
>  	return rc;
>  }
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-06  4:31 [ndctl PATCH v2 0/2] fix two issues reported by Coverity Zhiqiang Liu
2021-07-06  4:32 ` [ndctl PATCH v2 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
2021-07-06  4:33 ` [ndctl PATCH v2 2/2] namespace: Close fd before return in do_xaction_namespace() Zhiqiang Liu
2021-07-15  0:58   ` Zhiqiang Liu
2021-07-31  8:31   ` Zhiqiang Liu

NVDIMM Device and Persistent Memory development

Archives are clonable:
	git clone --mirror https://lore.kernel.org/nvdimm/0 nvdimm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 nvdimm nvdimm/ https://lore.kernel.org/nvdimm \
		nvdimm@lists.linux.dev
	public-inbox-index nvdimm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/dev.linux.lists.nvdimm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git