* [ndctl PATCH 0/2] fix two issues reported by Coverity
@ 2021-06-15 12:37 Zhiqiang Liu
2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Zhiqiang Liu @ 2021-06-15 12:37 UTC (permalink / raw)
To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26
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] 9+ messages in thread* [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace 2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu @ 2021-06-15 12:38 ` Zhiqiang Liu 2021-06-30 17:41 ` Alison Schofield 2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu 2021-06-30 2:55 ` [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu 2 siblings, 1 reply; 9+ messages in thread From: Zhiqiang Liu @ 2021-06-15 12:38 UTC (permalink / raw) To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26 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> --- 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 related [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace 2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu @ 2021-06-30 17:41 ` Alison Schofield 2021-07-01 3:23 ` Zhiqiang Liu 0 siblings, 1 reply; 9+ messages in thread From: Alison Schofield @ 2021-06-30 17:41 UTC (permalink / raw) To: Zhiqiang Liu; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng On Tue, Jun 15, 2021 at 08:38:33PM +0800, Zhiqiang Liu wrote: > > ndctl_pfn_get_namespace() may return NULL, so callers > should check return value of it. Otherwise, it may > cause access NULL pointer problem. > Hi Zhiqiang, I see you mentioned this was found by Coverity in the cover letter. Please repeat that in the commit log here. What about the call path: ndctl_dax_get_namespace() --> ndctl_pfn_get_namespace() Seems like another place where ndctl_pfn_get_namespace() could eventually lead to a NULL ptr dereference. Alison > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > --- > 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] 9+ messages in thread
* Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace 2021-06-30 17:41 ` Alison Schofield @ 2021-07-01 3:23 ` Zhiqiang Liu 2021-07-06 4:35 ` Zhiqiang Liu 0 siblings, 1 reply; 9+ messages in thread From: Zhiqiang Liu @ 2021-07-01 3:23 UTC (permalink / raw) To: Alison Schofield; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng On 2021/7/1 1:41, Alison Schofield wrote: > On Tue, Jun 15, 2021 at 08:38:33PM +0800, Zhiqiang Liu wrote: >> ndctl_pfn_get_namespace() may return NULL, so callers >> should check return value of it. Otherwise, it may >> cause access NULL pointer problem. >> > Hi Zhiqiang, > > I see you mentioned this was found by Coverity in the cover letter. > Please repeat that in the commit log here. > > What about the call path: > ndctl_dax_get_namespace() --> ndctl_pfn_get_namespace() > > Seems like another place where ndctl_pfn_get_namespace() could > eventually lead to a NULL ptr dereference. > > Alison Thanks for your reply. Call path: namespace_clear_bb -> if (dax) dax_clear_badblocks(dax) ->ndctl_dax_get_namespace(dax) ->ndctl_pfn_get_namespace(&dax->pfn) ->ndctl_pfn_get_ctx(pfn) struct ndctl_dax { struct ndctl_pfn pfn; struct daxctl_region *region; }; Here, we have checked that dax is not NULL before calling ndctl_dax_get_namespace(dax). If dax is not a NULL pointer, the dax->pfn will not be a NULL pointer. As for ndctl_pfn_get_ctx(pfn), it will access pfn->region->bus without NULL-checking, which may lead to a NULL ptr dereference as you said. I will correct it in the v2 patch. Regards, Zhiqiang Liu >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> --- >> 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] 9+ messages in thread
* Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace 2021-07-01 3:23 ` Zhiqiang Liu @ 2021-07-06 4:35 ` Zhiqiang Liu 0 siblings, 0 replies; 9+ messages in thread From: Zhiqiang Liu @ 2021-07-06 4:35 UTC (permalink / raw) To: Alison Schofield; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng On 2021/7/1 11:23, Zhiqiang Liu wrote: > > On 2021/7/1 1:41, Alison Schofield wrote: >> On Tue, Jun 15, 2021 at 08:38:33PM +0800, Zhiqiang Liu wrote: >>> ndctl_pfn_get_namespace() may return NULL, so callers >>> should check return value of it. Otherwise, it may >>> cause access NULL pointer problem. >>> >> Hi Zhiqiang, >> >> I see you mentioned this was found by Coverity in the cover letter. >> Please repeat that in the commit log here. >> >> What about the call path: >> ndctl_dax_get_namespace() --> ndctl_pfn_get_namespace() >> >> Seems like another place where ndctl_pfn_get_namespace() could >> eventually lead to a NULL ptr dereference. >> >> Alison > > Thanks for your reply. > > Call path: > > namespace_clear_bb > > -> if (dax) dax_clear_badblocks(dax) > > ->ndctl_dax_get_namespace(dax) > > ->ndctl_pfn_get_namespace(&dax->pfn) > > ->ndctl_pfn_get_ctx(pfn) > > > struct ndctl_dax { > struct ndctl_pfn pfn; > struct daxctl_region *region; > }; > > Here, we have checked that dax is not NULL before calling ndctl_dax_get_namespace(dax). > > If dax is not a NULL pointer, the dax->pfn will not be a NULL pointer. > > As for ndctl_pfn_get_ctx(pfn), it will access pfn->region->bus without NULL-checking, which > > may lead to a NULL ptr dereference as you said. I will correct it in the v2 patch. > Null_check in ndctl_pfn_get_**(pfn) is a common problem. I will try to solve this kind problem in new patches. Regards Zhiqiang Liu > > Regards, > > Zhiqiang Liu > >>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >>> --- >>> 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] 9+ messages in thread
* [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() 2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu 2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu @ 2021-06-15 12:39 ` Zhiqiang Liu 2021-06-30 17:47 ` Alison Schofield 2021-06-30 2:55 ` [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu 2 siblings, 1 reply; 9+ messages in thread From: Zhiqiang Liu @ 2021-06-15 12:39 UTC (permalink / raw) To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26 In do_xaction_namespace(), ri_ctx.f_out should be closed after being opened. Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> --- 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 related [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() 2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu @ 2021-06-30 17:47 ` Alison Schofield 2021-07-01 1:42 ` Zhiqiang Liu 0 siblings, 1 reply; 9+ messages in thread From: Alison Schofield @ 2021-06-30 17:47 UTC (permalink / raw) To: Zhiqiang Liu; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng On Tue, Jun 15, 2021 at 08:39:20PM +0800, Zhiqiang Liu wrote: > > In do_xaction_namespace(), ri_ctx.f_out should be closed after > being opened. > Hi Zhiqiang, The commit message and commit log need to be swapped. Something like: Commit message says what the patch does: [ndctl PATCH 2/2] namespace: Close fd before return in do_xaction_namespace() Commit log says why it needs to be done: This prevents a potential file descriptor leak in do_xaction_namespace() And, same as in Patch 1 - mention it was found by Coverity. Alison > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > --- > 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] 9+ messages in thread
* Re: [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() 2021-06-30 17:47 ` Alison Schofield @ 2021-07-01 1:42 ` Zhiqiang Liu 0 siblings, 0 replies; 9+ messages in thread From: Zhiqiang Liu @ 2021-07-01 1:42 UTC (permalink / raw) To: Alison Schofield; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng On 2021/7/1 1:47, Alison Schofield wrote: > On Tue, Jun 15, 2021 at 08:39:20PM +0800, Zhiqiang Liu wrote: >> In do_xaction_namespace(), ri_ctx.f_out should be closed after >> being opened. >> > Hi Zhiqiang, > > The commit message and commit log need to be swapped. > > Something like: > > Commit message says what the patch does: > [ndctl PATCH 2/2] namespace: Close fd before return in do_xaction_namespace() > > Commit log says why it needs to be done: > This prevents a potential file descriptor leak in do_xaction_namespace() > > And, same as in Patch 1 - mention it was found by Coverity. > > Alison Thanks for your advice. I will do that as your suggestion in v2 patch. > >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> --- >> 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] 9+ messages in thread
* Re: [ndctl PATCH 0/2] fix two issues reported by Coverity 2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu 2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu 2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu @ 2021-06-30 2:55 ` Zhiqiang Liu 2 siblings, 0 replies; 9+ messages in thread From: Zhiqiang Liu @ 2021-06-30 2:55 UTC (permalink / raw) To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26 friendly ping.. On 2021/6/15 20:37, Zhiqiang Liu wrote: > 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(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-06 4:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu 2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu 2021-06-30 17:41 ` Alison Schofield 2021-07-01 3:23 ` Zhiqiang Liu 2021-07-06 4:35 ` Zhiqiang Liu 2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu 2021-06-30 17:47 ` Alison Schofield 2021-07-01 1:42 ` Zhiqiang Liu 2021-06-30 2:55 ` [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.