[ndctl PATCH 0/8] fix serverl issues reported by Coverity

[ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Zhiqiang Liu]

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

lihaotian9 (8):
  namespace: check whether pfn|dax|btt is NULL in setup_namespace
  lib/libndctl: fix memory leakage problem in add_bus
  libdaxctl: fix memory leakage in add_dax_region()
  dimm: fix potential fd leakage in dimm_action()
  util/help: check whether strdup returns NULL in exec_man_konqueror
  lib/inject: check whether cmd is created successfully
  libndctl: check whether ndctl_btt_get_namespace returns NULL in
    callers
  namespace: check whether seed is NULL in validate_namespace_options

 daxctl/lib/libdaxctl.c |  3 +++
 ndctl/dimm.c           | 12 +++++++-----
 ndctl/lib/inject.c     |  8 ++++++++
 ndctl/lib/libndctl.c   |  1 +
 ndctl/namespace.c      | 23 ++++++++++++++++++-----
 test/libndctl.c        | 16 +++++++++++-----
 test/parent-uuid.c     |  2 +-
 util/help.c            |  8 +++++++-
 util/json.c            |  3 +++
 9 files changed, 59 insertions(+), 17 deletions(-)

-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 1/8] namespace: check whether pfn|dax|btt is NULL in setup_namespace

[Zhiqiang Liu]

In setup_namespace(), pfn|dax|btt is obtained by calling
ndctl_region_get_**_seed(), which may return NULL. So we
need to check whether pfn|dax|btt is NULL before accessing
them.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 ndctl/namespace.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index e946bb6..257384d 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -549,6 +549,8 @@ static int setup_namespace(struct ndctl_region *region,

 	if (do_setup_pfn(ndns, p)) {
 		struct ndctl_pfn *pfn = ndctl_region_get_pfn_seed(region);
+		if (!pfn)
+			return -ENXIO;

 		rc = check_dax_align(ndns);
 		if (rc)
@@ -563,6 +565,8 @@ static int setup_namespace(struct ndctl_region *region,
 			ndctl_pfn_set_namespace(pfn, NULL);
 	} else if (p->mode == NDCTL_NS_MODE_DEVDAX) {
 		struct ndctl_dax *dax = ndctl_region_get_dax_seed(region);
+		if (!dax)
+			return -ENXIO;

 		rc = check_dax_align(ndns);
 		if (rc)
@@ -577,7 +581,8 @@ static int setup_namespace(struct ndctl_region *region,
 			ndctl_dax_set_namespace(dax, NULL);
 	} else if (p->mode == NDCTL_NS_MODE_SECTOR) {
 		struct ndctl_btt *btt = ndctl_region_get_btt_seed(region);
-
+		if (!btt)
+			return -ENXIO;
 		/*
 		 * Handle the case of btt on a pmem namespace where the
 		 * pmem kernel support is pre-v1.2 namespace labels
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 2/8] lib/libndctl: fix memory leakage problem in add_bus

[Zhiqiang Liu]

In add_bus(), bus->bus_path is set by calling parent_dev_path(),
which will finally adopt realpath(, NULL) to allocate new path.
However, bus->bus_path will not be freed in err_read tag, then,
memory leakage occurs.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 ndctl/lib/libndctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ad521d3..3926382 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -975,6 +975,7 @@ static void *add_bus(void *parent, int id, const char *ctl_base)
 	free(bus->wait_probe_path);
 	free(bus->scrub_path);
 	free(bus->provider);
+	free(bus->bus_path);
 	free(bus->bus_buf);
 	free(bus);
  err_bus:
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 3/8] libdaxctl: fix memory leakage in add_dax_region()

[Zhiqiang Liu]

In add_dax_region(), region->devname is allocated by
calling strdup(), which may return NULL. So, we need
to check whether region->devname is NULL, and free
region->devname in err_read tag.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 daxctl/lib/libdaxctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index ee4a069..d841b78 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -287,6 +287,8 @@ static struct daxctl_region *add_dax_region(void *parent, int id,
 	region->refcount = 1;
 	list_head_init(&region->devices);
 	region->devname = strdup(devpath_to_devname(base));
+	if (!region->devname)
+		goto err_read;

 	sprintf(path, "%s/%s/size", base, attrs);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
@@ -314,6 +316,7 @@ static struct daxctl_region *add_dax_region(void *parent, int id,
  err_read:
 	free(region->region_buf);
 	free(region->region_path);
+	free(region->devname);
 	free(region);
  err_region:
 	free(path);
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 4/8] dimm: fix potential fd leakage in dimm_action()

[Zhiqiang Liu]

In dimm_action(), actx.f_out and actx.f_in may be set by calling
fopen(). If exceptions occur, we will directly goto out tag.
However, we did not close actx.f_out|actx.f_in in out tag, which
will cause fd leakage.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 ndctl/dimm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 90eb0b8..2f52cda 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -1352,7 +1352,7 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 			fprintf(stderr, "failed to open: %s: (%s)\n",
 					param.infile, strerror(errno));
 			rc = -errno;
-			goto out;
+			goto out_close_fout;
 		}
 	}

@@ -1371,7 +1371,7 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 		fprintf(stderr, "'%s' is not a valid label version\n",
 				param.labelversion);
 		rc = -EINVAL;
-		goto out;
+		goto out_close_fin_fout;
 	}

 	rc = 0;
@@ -1423,12 +1423,14 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 		util_display_json_array(actx.f_out, actx.jdimms, flags);
 	}

-	if (actx.f_out != stdout)
-		fclose(actx.f_out);
-
+ out_close_fin_fout:
 	if (actx.f_in != stdin)
 		fclose(actx.f_in);

+ out_close_fout:
+	if (actx.f_out != stdout)
+		fclose(actx.f_out);
+
  out:
 	/*
 	 * count if some actions succeeded, 0 if none were attempted,
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 5/8] util/help: check whether strdup returns NULL in exec_man_konqueror

[Zhiqiang Liu]

In exec_man_konqueror(), new is allocated by calling strdup(),
which may return NULL. We should check whether new is NULL before
using it.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 util/help.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/util/help.c b/util/help.c
index 2d57fa1..f867944 100644
--- a/util/help.c
+++ b/util/help.c
@@ -44,8 +44,14 @@ static void exec_man_konqueror(const char *path, const char *page)
 		if (path) {
 			const char *file = strrchr(path, '/');
 			if (file && !strcmp(file + 1, "konqueror")) {
+				char *dest;
 				char *new = strdup(path);
-				char *dest = strrchr(new, '/');
+				if (!new) {
+					pr_err("strdup(path) fails.\n");
+					exit(1);
+				}
+
+				dest = strrchr(new, '/');

 				/* strlen("konqueror") == strlen("kfmclient") */
 				strcpy(dest + 1, "kfmclient");
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 6/8] lib/inject: check whether cmd is created successfully

[Zhiqiang Liu]

ndctl_bus_cmd_new_ars_cp() is called to create cmd,
which may return NULL. We need to check whether it
is NULL in callers, such as ndctl_namespace_get_clear_uint
and ndctl_namespace_injection_status.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 ndctl/lib/inject.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
index 815f254..b543fc7 100644
--- a/ndctl/lib/inject.c
+++ b/ndctl/lib/inject.c
@@ -114,6 +114,10 @@ static int ndctl_namespace_get_clear_unit(struct ndctl_namespace *ndns)
 	if (rc)
 		return rc;
 	cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
+	if (!cmd) {
+		err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
+		return -ENOTTY;
+	}
 	rc = ndctl_cmd_submit(cmd);
 	if (rc < 0) {
 		dbg(ctx, "Error submitting ars_cap: %d\n", rc);
@@ -457,6 +461,10 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
 			return rc;

 		cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
+		if (!cmd) {
+			err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
+			return -ENOTTY;
+		}
 		rc = ndctl_cmd_submit(cmd);
 		if (rc < 0) {
 			dbg(ctx, "Error submitting ars_cap: %d\n", rc);
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 7/8] Check whether ndctl_btt_get_namespace returns NULL in callers

[Zhiqiang Liu]

ndctl_btt_get_namespace() may return NULL, so we need to check
return value of ndctl_btt_get_namespace() before using the return
value in callers.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 test/libndctl.c    | 16 +++++++++++-----
 test/parent-uuid.c |  2 +-
 util/json.c        |  3 +++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/test/libndctl.c b/test/libndctl.c
index 994e0fa..a8db388 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -983,13 +983,19 @@ static int check_pfn_create(struct ndctl_region *region,

 static int check_btt_size(struct ndctl_btt *btt)
 {
+	unsigned long long ns_size;
+	unsigned long sect_size;
+	unsigned long long actual, expect;
+	int size_select, sect_select;
 	struct ndctl_ctx *ctx = ndctl_btt_get_ctx(btt);
 	struct ndctl_test *test = ndctl_get_private_data(ctx);
 	struct ndctl_namespace *ndns = ndctl_btt_get_namespace(btt);
-	unsigned long long ns_size = ndctl_namespace_get_size(ndns);
-	unsigned long sect_size = ndctl_btt_get_sector_size(btt);
-	unsigned long long actual, expect;
-	int size_select, sect_select;
+
+	if (!ndns)
+		return -ENXIO;
+
+	ns_size = ndctl_namespace_get_size(ndns);
+	sect_size = ndctl_btt_get_sector_size(btt);
 	unsigned long long expect_table[][2] = {
 		[0] = {
 			[0] = 0x11b5400,
@@ -1461,7 +1467,7 @@ static int check_btt_autodetect(struct ndctl_bus *bus,
 		if (!ndctl_btt_is_enabled(btt))
 			continue;
 		btt_ndns = ndctl_btt_get_namespace(btt);
-		if (strcmp(ndctl_namespace_get_devname(btt_ndns), devname) != 0)
+		if (!btt_ndns || strcmp(ndctl_namespace_get_devname(btt_ndns), devname) != 0)
 			continue;
 		fprintf(stderr, "%s: btt_ndns: %p ndns: %p\n", __func__,
 				btt_ndns, ndns);
diff --git a/test/parent-uuid.c b/test/parent-uuid.c
index f41ca2c..303b30b 100644
--- a/test/parent-uuid.c
+++ b/test/parent-uuid.c
@@ -115,7 +115,7 @@ static struct ndctl_btt *check_valid_btt(struct ndctl_region *region,
 		if (!ndctl_btt_is_enabled(btt))
 			continue;
 		btt_ndns = ndctl_btt_get_namespace(btt);
-		if (strcmp(ndctl_namespace_get_devname(btt_ndns),
+		if (!btt_ndns || strcmp(ndctl_namespace_get_devname(btt_ndns),
 				ndctl_namespace_get_devname(ndns)) != 0)
 			continue;
 		return btt;
diff --git a/util/json.c b/util/json.c
index 77bd478..1392403 100644
--- a/util/json.c
+++ b/util/json.c
@@ -1004,6 +1004,9 @@ static void util_btt_badblocks_to_json(struct ndctl_btt *btt,
 	struct ndctl_namespace *ndns = ndctl_btt_get_namespace(btt);
 	unsigned long long begin, size;

+	if (!ndns)
+		return;
+
 	begin = ndctl_namespace_get_resource(ndns);
 	if (begin == ULLONG_MAX)
 		return;
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH 8/8] Check whether seed is NULL in validate_namespace_options

[Zhiqiang Liu]

In validate_namespace_options(), seed is obtained through
calling ndctl_region_get_namespace_seed(), which may return
NULL. If seed is NULL, we should return directly with error
code. In addition, we can use region_name var rather than
calling ndctl_region_get_devname() again.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 ndctl/namespace.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 257384d..5a92d62 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -847,8 +847,7 @@ static int validate_namespace_options(struct ndctl_region *region,
 	region_align = ndctl_region_get_align(region);
 	if (region_align < ULONG_MAX && p->size % region_align) {
 		err("%s: align setting is %#lx size %#llx is misaligned\n",
-				ndctl_region_get_devname(region), region_align,
-				p->size);
+				region_name, region_align, p->size);
 		return -EINVAL;
 	}

@@ -924,8 +923,13 @@ static int validate_namespace_options(struct ndctl_region *region,
 		} else {
 			struct ndctl_namespace *seed = ndns;

-			if (!seed)
+			if (!seed) {
 				seed = ndctl_region_get_namespace_seed(region);
+				if (!seed) {
+					err("%s: failed to get seed\n", region_name);
+					return -ENXIO;
+				}
+			}
 			num = ndctl_namespace_get_num_sector_sizes(seed);
 			for (i = 0; i < num; i++)
 				if (ndctl_namespace_get_supported_sector_size(seed, i)
@@ -953,9 +957,13 @@ static int validate_namespace_options(struct ndctl_region *region,
 		struct ndctl_namespace *seed;

 		seed = ndctl_region_get_namespace_seed(region);
+		if (!seed) {
+			err("%s: failed to get seed\n", region_name);
+			return -ENXIO;
+		}
 		if (ndctl_namespace_get_type(seed) == ND_DEVICE_NAMESPACE_BLK)
 			debug("%s: set_defaults() should preclude this?\n",
-				ndctl_region_get_devname(region));
+				region_name);
 		/*
 		 * Pick a default sector size for a pmem namespace based
 		 * on what the kernel supports.
-- 
1.8.3.1


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Zhiqiang Liu]

friendly ping...

On 2020/11/6 17:23, Zhiqiang Liu wrote:
> 
> Recently, we use Coverity to analysis the ndctl package.
> Several issues should be resolved to make Coverity happy.
> 
> lihaotian9 (8):
>   namespace: check whether pfn|dax|btt is NULL in setup_namespace
>   lib/libndctl: fix memory leakage problem in add_bus
>   libdaxctl: fix memory leakage in add_dax_region()
>   dimm: fix potential fd leakage in dimm_action()
>   util/help: check whether strdup returns NULL in exec_man_konqueror
>   lib/inject: check whether cmd is created successfully
>   libndctl: check whether ndctl_btt_get_namespace returns NULL in
>     callers
>   namespace: check whether seed is NULL in validate_namespace_options
> 
>  daxctl/lib/libdaxctl.c |  3 +++
>  ndctl/dimm.c           | 12 +++++++-----
>  ndctl/lib/inject.c     |  8 ++++++++
>  ndctl/lib/libndctl.c   |  1 +
>  ndctl/namespace.c      | 23 ++++++++++++++++++-----
>  test/libndctl.c        | 16 +++++++++++-----
>  test/parent-uuid.c     |  2 +-
>  util/help.c            |  8 +++++++-
>  util/json.c            |  3 +++
>  9 files changed, 59 insertions(+), 17 deletions(-)
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Zhiqiang Liu]

Friendly ping...

I just wonder if this kind of patches will not be reviewed
and processed.

I`d be very happy to receive any feedback.

On 2020/11/6 17:23, Zhiqiang Liu wrote:
> 
> Recently, we use Coverity to analysis the ndctl package.
> Several issues should be resolved to make Coverity happy.
> 
> lihaotian9 (8):
>   namespace: check whether pfn|dax|btt is NULL in setup_namespace
>   lib/libndctl: fix memory leakage problem in add_bus
>   libdaxctl: fix memory leakage in add_dax_region()
>   dimm: fix potential fd leakage in dimm_action()
>   util/help: check whether strdup returns NULL in exec_man_konqueror
>   lib/inject: check whether cmd is created successfully
>   libndctl: check whether ndctl_btt_get_namespace returns NULL in
>     callers
>   namespace: check whether seed is NULL in validate_namespace_options
> 
>  daxctl/lib/libdaxctl.c |  3 +++
>  ndctl/dimm.c           | 12 +++++++-----
>  ndctl/lib/inject.c     |  8 ++++++++
>  ndctl/lib/libndctl.c   |  1 +
>  ndctl/namespace.c      | 23 ++++++++++++++++++-----
>  test/libndctl.c        | 16 +++++++++++-----
>  test/parent-uuid.c     |  2 +-
>  util/help.c            |  8 +++++++-
>  util/json.c            |  3 +++
>  9 files changed, 59 insertions(+), 17 deletions(-)
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Verma, Vishal L]

On Fri, 2020-11-20 at 10:45 +0800, Zhiqiang Liu wrote:
> Friendly ping...
> 
> I just wonder if this kind of patches will not be reviewed
> and processed.
> 
> I`d be very happy to receive any feedback.

Hi Zhiqiang,

These are definitely on my list to look at, I've just not had the time
to do so yet. I'm hoping to clear up my ndctl backlog in the next couple
of weeks though and get a v71 release out.

Thanks for the patience!

-Vishal

> 
> On 2020/11/6 17:23, Zhiqiang Liu wrote:
> > Recently, we use Coverity to analysis the ndctl package.
> > Several issues should be resolved to make Coverity happy.
> > 
> > lihaotian9 (8):
> >   namespace: check whether pfn|dax|btt is NULL in setup_namespace
> >   lib/libndctl: fix memory leakage problem in add_bus
> >   libdaxctl: fix memory leakage in add_dax_region()
> >   dimm: fix potential fd leakage in dimm_action()
> >   util/help: check whether strdup returns NULL in exec_man_konqueror
> >   lib/inject: check whether cmd is created successfully
> >   libndctl: check whether ndctl_btt_get_namespace returns NULL in
> >     callers
> >   namespace: check whether seed is NULL in validate_namespace_options
> > 
> >  daxctl/lib/libdaxctl.c |  3 +++
> >  ndctl/dimm.c           | 12 +++++++-----
> >  ndctl/lib/inject.c     |  8 ++++++++
> >  ndctl/lib/libndctl.c   |  1 +
> >  ndctl/namespace.c      | 23 ++++++++++++++++++-----
> >  test/libndctl.c        | 16 +++++++++++-----
> >  test/parent-uuid.c     |  2 +-
> >  util/help.c            |  8 +++++++-
> >  util/json.c            |  3 +++
> >  9 files changed, 59 insertions(+), 17 deletions(-)
> > 
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Zhiqiang Liu]

On 2020/11/20 10:47, Verma, Vishal L wrote:
> On Fri, 2020-11-20 at 10:45 +0800, Zhiqiang Liu wrote:
>> Friendly ping...
>>
>> I just wonder if this kind of patches will not be reviewed
>> and processed.
>>
>> I`d be very happy to receive any feedback.
> 
> Hi Zhiqiang,
> 
> These are definitely on my list to look at, I've just not had the time
> to do so yet. I'm hoping to clear up my ndctl backlog in the next couple
> of weeks though and get a v71 release out.
> 
> Thanks for the patience!
> 
> -Vishal
> 

Thanks for your reply.
Looking forward to the new release.

Thank you again.

-Zhiqiang Liu

>>
>> On 2020/11/6 17:23, Zhiqiang Liu wrote:
>>> Recently, we use Coverity to analysis the ndctl package.
>>> Several issues should be resolved to make Coverity happy.
>>>
>>> lihaotian9 (8):
>>>   namespace: check whether pfn|dax|btt is NULL in setup_namespace
>>>   lib/libndctl: fix memory leakage problem in add_bus
>>>   libdaxctl: fix memory leakage in add_dax_region()
>>>   dimm: fix potential fd leakage in dimm_action()
>>>   util/help: check whether strdup returns NULL in exec_man_konqueror
>>>   lib/inject: check whether cmd is created successfully
>>>   libndctl: check whether ndctl_btt_get_namespace returns NULL in
>>>     callers
>>>   namespace: check whether seed is NULL in validate_namespace_options
>>>
>>>  daxctl/lib/libdaxctl.c |  3 +++
>>>  ndctl/dimm.c           | 12 +++++++-----
>>>  ndctl/lib/inject.c     |  8 ++++++++
>>>  ndctl/lib/libndctl.c   |  1 +
>>>  ndctl/namespace.c      | 23 ++++++++++++++++++-----
>>>  test/libndctl.c        | 16 +++++++++++-----
>>>  test/parent-uuid.c     |  2 +-
>>>  util/help.c            |  8 +++++++-
>>>  util/json.c            |  3 +++
>>>  9 files changed, 59 insertions(+), 17 deletions(-)
>>>
>> _______________________________________________
>> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
>> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
> 
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
> 
> .
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 1/8] namespace: check whether pfn|dax|btt is NULL in setup_namespace

[Jeff Moyer]

Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:

> In setup_namespace(), pfn|dax|btt is obtained by calling
> ndctl_region_get_**_seed(), which may return NULL. So we
> need to check whether pfn|dax|btt is NULL before accessing
> them.
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  ndctl/namespace.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index e946bb6..257384d 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -549,6 +549,8 @@ static int setup_namespace(struct ndctl_region *region,
>
>  	if (do_setup_pfn(ndns, p)) {
>  		struct ndctl_pfn *pfn = ndctl_region_get_pfn_seed(region);
> +		if (!pfn)
> +			return -ENXIO;
>
>  		rc = check_dax_align(ndns);
>  		if (rc)
> @@ -563,6 +565,8 @@ static int setup_namespace(struct ndctl_region *region,
>  			ndctl_pfn_set_namespace(pfn, NULL);
>  	} else if (p->mode == NDCTL_NS_MODE_DEVDAX) {
>  		struct ndctl_dax *dax = ndctl_region_get_dax_seed(region);
> +		if (!dax)
> +			return -ENXIO;
>
>  		rc = check_dax_align(ndns);
>  		if (rc)
> @@ -577,7 +581,8 @@ static int setup_namespace(struct ndctl_region *region,
>  			ndctl_dax_set_namespace(dax, NULL);
>  	} else if (p->mode == NDCTL_NS_MODE_SECTOR) {
>  		struct ndctl_btt *btt = ndctl_region_get_btt_seed(region);
> -
> +		if (!btt)
> +			return -ENXIO;
>  		/*
>  		 * Handle the case of btt on a pmem namespace where the
>  		 * pmem kernel support is pre-v1.2 namespace labels

Minor inconsistency in the last hunk.  The empty line should come after
the return, no?

Other than that, LGTM.

Acked-by: Jeff Moyer <jmoyer@redhat.com>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 2/8] lib/libndctl: fix memory leakage problem in add_bus

[Jeff Moyer]

Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:

> In add_bus(), bus->bus_path is set by calling parent_dev_path(),
> which will finally adopt realpath(, NULL) to allocate new path.
> However, bus->bus_path will not be freed in err_read tag, then,
> memory leakage occurs.
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  ndctl/lib/libndctl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ad521d3..3926382 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -975,6 +975,7 @@ static void *add_bus(void *parent, int id, const char *ctl_base)
>  	free(bus->wait_probe_path);
>  	free(bus->scrub_path);
>  	free(bus->provider);
> +	free(bus->bus_path);
>  	free(bus->bus_buf);
>  	free(bus);
>   err_bus:

Acked-by: Jeff Moyer <jmoyer@redhat.com>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 3/8] libdaxctl: fix memory leakage in add_dax_region()

[Jeff Moyer]

Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:

> In add_dax_region(), region->devname is allocated by
> calling strdup(), which may return NULL. So, we need
> to check whether region->devname is NULL, and free
> region->devname in err_read tag.
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  daxctl/lib/libdaxctl.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index ee4a069..d841b78 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -287,6 +287,8 @@ static struct daxctl_region *add_dax_region(void *parent, int id,
>  	region->refcount = 1;
>  	list_head_init(&region->devices);
>  	region->devname = strdup(devpath_to_devname(base));
> +	if (!region->devname)
> +		goto err_read;
>
>  	sprintf(path, "%s/%s/size", base, attrs);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
> @@ -314,6 +316,7 @@ static struct daxctl_region *add_dax_region(void *parent, int id,
>   err_read:
>  	free(region->region_buf);
>  	free(region->region_path);
> +	free(region->devname);
>  	free(region);
>   err_region:
>  	free(path);

Acked-by: Jeff Moyer <jmoyer@redhat.com>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 4/8] dimm: fix potential fd leakage in dimm_action()

[Jeff Moyer]

Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:

> In dimm_action(), actx.f_out and actx.f_in may be set by calling
> fopen(). If exceptions occur, we will directly goto out tag.
> However, we did not close actx.f_out|actx.f_in in out tag, which
> will cause fd leakage.
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  ndctl/dimm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> index 90eb0b8..2f52cda 100644
> --- a/ndctl/dimm.c
> +++ b/ndctl/dimm.c
> @@ -1352,7 +1352,7 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
>  			fprintf(stderr, "failed to open: %s: (%s)\n",
>  					param.infile, strerror(errno));
>  			rc = -errno;
> -			goto out;
> +			goto out_close_fout;
>  		}
>  	}
>
> @@ -1371,7 +1371,7 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
>  		fprintf(stderr, "'%s' is not a valid label version\n",
>  				param.labelversion);
>  		rc = -EINVAL;
> -		goto out;
> +		goto out_close_fin_fout;
>  	}
>
>  	rc = 0;
> @@ -1423,12 +1423,14 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
>  		util_display_json_array(actx.f_out, actx.jdimms, flags);
>  	}
>
> -	if (actx.f_out != stdout)
> -		fclose(actx.f_out);
> -
> + out_close_fin_fout:
>  	if (actx.f_in != stdin)
>  		fclose(actx.f_in);
>
> + out_close_fout:
> +	if (actx.f_out != stdout)
> +		fclose(actx.f_out);
> +
>   out:
>  	/*
>  	 * count if some actions succeeded, 0 if none were attempted,

Acked-by: Jeff Moyer <jmoyer@redhat.com>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Jeff Moyer]

Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:

> Recently, we use Coverity to analysis the ndctl package.
> Several issues should be resolved to make Coverity happy.
>
> lihaotian9 (8):
>   namespace: check whether pfn|dax|btt is NULL in setup_namespace
>   lib/libndctl: fix memory leakage problem in add_bus
>   libdaxctl: fix memory leakage in add_dax_region()
>   dimm: fix potential fd leakage in dimm_action()
>   util/help: check whether strdup returns NULL in exec_man_konqueror
>   lib/inject: check whether cmd is created successfully
>   libndctl: check whether ndctl_btt_get_namespace returns NULL in
>     callers
>   namespace: check whether seed is NULL in validate_namespace_options
>
>  daxctl/lib/libdaxctl.c |  3 +++
>  ndctl/dimm.c           | 12 +++++++-----
>  ndctl/lib/inject.c     |  8 ++++++++
>  ndctl/lib/libndctl.c   |  1 +
>  ndctl/namespace.c      | 23 ++++++++++++++++++-----
>  test/libndctl.c        | 16 +++++++++++-----
>  test/parent-uuid.c     |  2 +-
>  util/help.c            |  8 +++++++-
>  util/json.c            |  3 +++
>  9 files changed, 59 insertions(+), 17 deletions(-)

Except for the minor nit I pointed out, for the  series:

Acked-by: Jeff Moyer <jmoyer@redhat.com>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity

[Zhiqiang Liu]

Thanks for your reply.
I will add one empty line in 1/8 patch in v2.

Regards
Zhiqiang Liu

On 2020/11/21 0:45, Jeff Moyer wrote:
> Zhiqiang Liu <liuzhiqiang26@huawei.com> writes:
> 
>> Recently, we use Coverity to analysis the ndctl package.
>> Several issues should be resolved to make Coverity happy.
>>
>> lihaotian9 (8):
>>   namespace: check whether pfn|dax|btt is NULL in setup_namespace
>>   lib/libndctl: fix memory leakage problem in add_bus
>>   libdaxctl: fix memory leakage in add_dax_region()
>>   dimm: fix potential fd leakage in dimm_action()
>>   util/help: check whether strdup returns NULL in exec_man_konqueror
>>   lib/inject: check whether cmd is created successfully
>>   libndctl: check whether ndctl_btt_get_namespace returns NULL in
>>     callers
>>   namespace: check whether seed is NULL in validate_namespace_options
>>
>>  daxctl/lib/libdaxctl.c |  3 +++
>>  ndctl/dimm.c           | 12 +++++++-----
>>  ndctl/lib/inject.c     |  8 ++++++++
>>  ndctl/lib/libndctl.c   |  1 +
>>  ndctl/namespace.c      | 23 ++++++++++++++++++-----
>>  test/libndctl.c        | 16 +++++++++++-----
>>  test/parent-uuid.c     |  2 +-
>>  util/help.c            |  8 +++++++-
>>  util/json.c            |  3 +++
>>  9 files changed, 59 insertions(+), 17 deletions(-)
> 
> Except for the minor nit I pointed out, for the  series:
> 
> Acked-by: Jeff Moyer <jmoyer@redhat.com>
> 
> 
> .
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 6/8] lib/inject: check whether cmd is created successfully

[Verma, Vishal L]

On Fri, 2020-11-06 at 17:27 +0800, Zhiqiang Liu wrote:
> ndctl_bus_cmd_new_ars_cp() is called to create cmd,
> which may return NULL. We need to check whether it
> is NULL in callers, such as ndctl_namespace_get_clear_uint
> and ndctl_namespace_injection_status.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
>  ndctl/lib/inject.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
> index 815f254..b543fc7 100644
> --- a/ndctl/lib/inject.c
> +++ b/ndctl/lib/inject.c
> @@ -114,6 +114,10 @@ static int ndctl_namespace_get_clear_unit(struct ndctl_namespace *ndns)
>  	if (rc)
>  		return rc;
>  	cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
> +	if (!cmd) {
> +		err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
> +		return -ENOTTY;
> +	}
>  	rc = ndctl_cmd_submit(cmd);
>  	if (rc < 0) {
>  		dbg(ctx, "Error submitting ars_cap: %d\n", rc);
> @@ -457,6 +461,10 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
>  			return rc;
> 
>  		cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
> +		if (!cmd) {
> +			err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
> +			return -ENOTTY;
> +		}
>  		rc = ndctl_cmd_submit(cmd);
>  		if (rc < 0) {
>  			dbg(ctx, "Error submitting ars_cap: %d\n", rc);

This looks good in general, but I made some small fixups while applying.
Printing the bus provider here isn't as useful - I replaced it with
printing the namespace 'devname':

-               err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
+               err(ctx, "%s: failed to create cmd\n",
+                       ndctl_namespace_get_devname(ndns));

Also fixed up a couple of typos in commit messages, but otherwise the
series looks good and I've applied it for v71.

Thanks,
-Vishal
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH 6/8] lib/inject: check whether cmd is created successfully

[Zhiqiang Liu]

On 2020/12/17 11:33, Verma, Vishal L wrote:
> On Fri, 2020-11-06 at 17:27 +0800, Zhiqiang Liu wrote:
>> ndctl_bus_cmd_new_ars_cp() is called to create cmd,
>> which may return NULL. We need to check whether it
>> is NULL in callers, such as ndctl_namespace_get_clear_uint
>> and ndctl_namespace_injection_status.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>>  ndctl/lib/inject.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
>> index 815f254..b543fc7 100644
>> --- a/ndctl/lib/inject.c
>> +++ b/ndctl/lib/inject.c
>> @@ -114,6 +114,10 @@ static int ndctl_namespace_get_clear_unit(struct ndctl_namespace *ndns)
>>  	if (rc)
>>  		return rc;
>>  	cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
>> +	if (!cmd) {
>> +		err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
>> +		return -ENOTTY;
>> +	}
>>  	rc = ndctl_cmd_submit(cmd);
>>  	if (rc < 0) {
>>  		dbg(ctx, "Error submitting ars_cap: %d\n", rc);
>> @@ -457,6 +461,10 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
>>  			return rc;
>>
>>  		cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
>> +		if (!cmd) {
>> +			err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
>> +			return -ENOTTY;
>> +		}
>>  		rc = ndctl_cmd_submit(cmd);
>>  		if (rc < 0) {
>>  			dbg(ctx, "Error submitting ars_cap: %d\n", rc);
> 
> This looks good in general, but I made some small fixups while applying.
> Printing the bus provider here isn't as useful - I replaced it with
> printing the namespace 'devname':
> 
> -               err(ctx, "bus: %s failed to create cmd\n", ndctl_bus_get_provider(bus));
> +               err(ctx, "%s: failed to create cmd\n",
> +                       ndctl_namespace_get_devname(ndns));
> 
> Also fixed up a couple of typos in commit messages, but otherwise the
> series looks good and I've applied it for v71.
> 

Thanks again.

Regards
Zhiqiang Liu

> Thanks,
> -Vishal
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org