* [PATCH v3] Monitor: use devname as char array instead of pointer
@ 2022-06-20 14:06 Kinga Tanska
2022-06-25 7:03 ` Coly Li
0 siblings, 1 reply; 4+ messages in thread
From: Kinga Tanska @ 2022-06-20 14:06 UTC (permalink / raw)
To: linux-raid; +Cc: jes, colyli
Device name wasn't filled properly due to incorrect use of strcpy.
Strcpy was used twice. Firstly to fill devname with "/dev/md/"
and then to add chosen name. First strcpy result was overwritten by
second one (as a result <device_name> instead of "/dev/md/<device_name>"
was assigned). This commit changes this implementation to use snprintf
and devname with fixed size. Also safer string functions are propagated.
Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
---
Monitor.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/Monitor.c b/Monitor.c
index 6ca1ebe5..04112791 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -33,8 +33,8 @@
#endif
struct state {
- char *devname;
- char devnm[32]; /* to sync with mdstat info */
+ char devname[MD_NAME_MAX + 8];
+ char devnm[MD_NAME_MAX]; /* to sync with mdstat info */
unsigned int utime;
int err;
char *spare_group;
@@ -45,7 +45,7 @@ struct state {
int devstate[MAX_DISKS];
dev_t devid[MAX_DISKS];
int percent;
- char parent_devnm[32]; /* For subarray, devnm of parent.
+ char parent_devnm[MD_NAME_MAX]; /* For subarray, devnm of parent.
* For others, ""
*/
struct supertype *metadata;
@@ -187,13 +187,7 @@ int Monitor(struct mddev_dev *devlist,
continue;
st = xcalloc(1, sizeof *st);
- if (mdlist->devname[0] == '/')
- st->devname = xstrdup(mdlist->devname);
- else {
- st->devname = xmalloc(8+strlen(mdlist->devname)+1);
- strcpy(strcpy(st->devname, "/dev/md/"),
- mdlist->devname);
- }
+ snprintf(st->devname, MD_NAME_MAX + 8, "/dev/md/%s", basename(mdlist->devname));
if (!is_mddev(mdlist->devname))
return 1;
st->next = statelist;
@@ -216,7 +210,7 @@ int Monitor(struct mddev_dev *devlist,
st = xcalloc(1, sizeof *st);
mdlist = conf_get_ident(dv->devname);
- st->devname = xstrdup(dv->devname);
+ snprintf(st->devname, MD_NAME_MAX + 8, "%s", dv->devname);
st->next = statelist;
st->devnm[0] = 0;
st->percent = RESYNC_UNKNOWN;
@@ -299,7 +293,6 @@ int Monitor(struct mddev_dev *devlist,
for (stp = &statelist; (st = *stp) != NULL; ) {
if (st->from_auto && st->err > 5) {
*stp = st->next;
- free(st->devname);
free(st->spare_group);
free(st);
} else
@@ -552,7 +545,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
goto disappeared;
if (st->devnm[0] == 0)
- strcpy(st->devnm, fd2devnm(fd));
+ snprintf(st->devnm, MD_NAME_MAX, "%s", fd2devnm(fd));
for (mse2 = mdstat; mse2; mse2 = mse2->next)
if (strcmp(mse2->devnm, st->devnm) == 0) {
@@ -682,7 +675,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
strncmp(mse->metadata_version, "external:", 9) == 0 &&
is_subarray(mse->metadata_version+9)) {
char *sl;
- strcpy(st->parent_devnm, mse->metadata_version + 10);
+ snprintf(st->parent_devnm, MD_NAME_MAX, "%s", mse->metadata_version + 10);
sl = strchr(st->parent_devnm, '/');
if (sl)
*sl = 0;
@@ -770,14 +763,13 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
continue;
}
- st->devname = xstrdup(name);
+ snprintf(st->devname, MD_NAME_MAX + 8, "%s", name);
if ((fd = open(st->devname, O_RDONLY)) < 0 ||
md_get_array_info(fd, &array) < 0) {
/* no such array */
if (fd >= 0)
close(fd);
put_md_name(st->devname);
- free(st->devname);
if (st->metadata) {
st->metadata->ss->free_super(st->metadata);
free(st->metadata);
@@ -789,7 +781,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
st->next = *statelist;
st->err = 1;
st->from_auto = 1;
- strcpy(st->devnm, mse->devnm);
+ snprintf(st->devnm, MD_NAME_MAX, "%s", mse->devnm);
st->percent = RESYNC_UNKNOWN;
st->expected_spares = -1;
if (mse->metadata_version &&
@@ -797,8 +789,8 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
"external:", 9) == 0 &&
is_subarray(mse->metadata_version+9)) {
char *sl;
- strcpy(st->parent_devnm,
- mse->metadata_version+10);
+ snprintf(st->parent_devnm, MD_NAME_MAX,
+ "%s", mse->metadata_version + 10);
sl = strchr(st->parent_devnm, '/');
*sl = 0;
} else
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] Monitor: use devname as char array instead of pointer
2022-06-20 14:06 [PATCH v3] Monitor: use devname as char array instead of pointer Kinga Tanska
@ 2022-06-25 7:03 ` Coly Li
2022-07-05 12:05 ` Kinga Tanska
0 siblings, 1 reply; 4+ messages in thread
From: Coly Li @ 2022-06-25 7:03 UTC (permalink / raw)
To: Kinga Tanska; +Cc: linux-raid, jes
> 2022年6月20日 22:06，Kinga Tanska <kinga.tanska@intel.com> 写道：
>
> Device name wasn't filled properly due to incorrect use of strcpy.
> Strcpy was used twice. Firstly to fill devname with "/dev/md/"
> and then to add chosen name. First strcpy result was overwritten by
> second one (as a result <device_name> instead of "/dev/md/<device_name>"
> was assigned). This commit changes this implementation to use snprintf
> and devname with fixed size. Also safer string functions are propagated.
>
> Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
> ---
> Monitor.c | 30 +++++++++++-------------------
> 1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index 6ca1ebe5..04112791 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -33,8 +33,8 @@
> #endif
>
> struct state {
> - char *devname;
> - char devnm[32]; /* to sync with mdstat info */
> + char devname[MD_NAME_MAX + 8];
> + char devnm[MD_NAME_MAX]; /* to sync with mdstat info */
> unsigned int utime;
> int err;
> char *spare_group;
> @@ -45,7 +45,7 @@ struct state {
> int devstate[MAX_DISKS];
> dev_t devid[MAX_DISKS];
> int percent;
> - char parent_devnm[32]; /* For subarray, devnm of parent.
> + char parent_devnm[MD_NAME_MAX]; /* For subarray, devnm of parent.
> * For others, ""
> */
> struct supertype *metadata;
> @@ -187,13 +187,7 @@ int Monitor(struct mddev_dev *devlist,
> continue;
>
> st = xcalloc(1, sizeof *st);
> - if (mdlist->devname[0] == '/')
> - st->devname = xstrdup(mdlist->devname);
> - else {
> - st->devname = xmalloc(8+strlen(mdlist->devname)+1);
> - strcpy(strcpy(st->devname, "/dev/md/"),
> - mdlist->devname);
> - }
> + snprintf(st->devname, MD_NAME_MAX + 8, "/dev/md/%s", basename(mdlist->devname));
Hi Kinga,
The above location is what the whole patch wants to fix. It is not worth to change other locations. IMHO the change set is a bit large than what it should be, just a suggestion maybe a following style change is enough (not test it),
@@ -190,9 +190,13 @@ int Monitor(struct mddev_dev *devlist,
if (mdlist->devname[0] == '/')
st->devname = xstrdup(mdlist->devname);
else {
- st->devname = xmalloc(8+strlen(mdlist->devname)+1);
- strcpy(strcpy(st->devname, "/dev/md/"),
- mdlist->devname);
+ int _len;
+
+ _len = 8 + strlen(mdlist->devname);
+ st->devname = xmalloc(_len + 1);
+ memset(st->devname, 0, _len + 1);
+ snprintf(st->devname, _len, "/dev/md/%s",
+ mdlist->devname);
}
if (!is_mddev(mdlist->devname))
return 1;
Thanks.
Coly Li
> if (!is_mddev(mdlist->devname))
> return 1;
> st->next = statelist;
> @@ -216,7 +210,7 @@ int Monitor(struct mddev_dev *devlist,
>
> st = xcalloc(1, sizeof *st);
> mdlist = conf_get_ident(dv->devname);
> - st->devname = xstrdup(dv->devname);
> + snprintf(st->devname, MD_NAME_MAX + 8, "%s", dv->devname);
> st->next = statelist;
> st->devnm[0] = 0;
> st->percent = RESYNC_UNKNOWN;
> @@ -299,7 +293,6 @@ int Monitor(struct mddev_dev *devlist,
> for (stp = &statelist; (st = *stp) != NULL; ) {
> if (st->from_auto && st->err > 5) {
> *stp = st->next;
> - free(st->devname);
> free(st->spare_group);
> free(st);
> } else
> @@ -552,7 +545,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
> goto disappeared;
>
> if (st->devnm[0] == 0)
> - strcpy(st->devnm, fd2devnm(fd));
> + snprintf(st->devnm, MD_NAME_MAX, "%s", fd2devnm(fd));
>
> for (mse2 = mdstat; mse2; mse2 = mse2->next)
> if (strcmp(mse2->devnm, st->devnm) == 0) {
> @@ -682,7 +675,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
> strncmp(mse->metadata_version, "external:", 9) == 0 &&
> is_subarray(mse->metadata_version+9)) {
> char *sl;
> - strcpy(st->parent_devnm, mse->metadata_version + 10);
> + snprintf(st->parent_devnm, MD_NAME_MAX, "%s", mse->metadata_version + 10);
> sl = strchr(st->parent_devnm, '/');
> if (sl)
> *sl = 0;
> @@ -770,14 +763,13 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
> continue;
> }
>
> - st->devname = xstrdup(name);
> + snprintf(st->devname, MD_NAME_MAX + 8, "%s", name);
> if ((fd = open(st->devname, O_RDONLY)) < 0 ||
> md_get_array_info(fd, &array) < 0) {
> /* no such array */
> if (fd >= 0)
> close(fd);
> put_md_name(st->devname);
> - free(st->devname);
> if (st->metadata) {
> st->metadata->ss->free_super(st->metadata);
> free(st->metadata);
> @@ -789,7 +781,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
> st->next = *statelist;
> st->err = 1;
> st->from_auto = 1;
> - strcpy(st->devnm, mse->devnm);
> + snprintf(st->devnm, MD_NAME_MAX, "%s", mse->devnm);
> st->percent = RESYNC_UNKNOWN;
> st->expected_spares = -1;
> if (mse->metadata_version &&
> @@ -797,8 +789,8 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
> "external:", 9) == 0 &&
> is_subarray(mse->metadata_version+9)) {
> char *sl;
> - strcpy(st->parent_devnm,
> - mse->metadata_version+10);
> + snprintf(st->parent_devnm, MD_NAME_MAX,
> + "%s", mse->metadata_version + 10);
> sl = strchr(st->parent_devnm, '/');
> *sl = 0;
> } else
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] Monitor: use devname as char array instead of pointer
2022-06-25 7:03 ` Coly Li
@ 2022-07-05 12:05 ` Kinga Tanska
2022-07-05 14:06 ` Coly Li
0 siblings, 1 reply; 4+ messages in thread
From: Kinga Tanska @ 2022-07-05 12:05 UTC (permalink / raw)
To: Coly Li; +Cc: linux-raid, jes
On Sat, 25 Jun 2022 15:03:32 +0800
Coly Li <colyli@suse.de> wrote:
> > 2022年6月20日 22:06，Kinga Tanska <kinga.tanska@intel.com> 写道：
> >
> > Device name wasn't filled properly due to incorrect use of strcpy.
> > Strcpy was used twice. Firstly to fill devname with "/dev/md/"
> > and then to add chosen name. First strcpy result was overwritten by
> > second one (as a result <device_name> instead of
> > "/dev/md/<device_name>" was assigned). This commit changes this
> > implementation to use snprintf and devname with fixed size. Also
> > safer string functions are propagated.
> >
> > Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
> > ---
> > Monitor.c | 30 +++++++++++-------------------
> > 1 file changed, 11 insertions(+), 19 deletions(-)
> >
> > diff --git a/Monitor.c b/Monitor.c
> > index 6ca1ebe5..04112791 100644
> > --- a/Monitor.c
> > +++ b/Monitor.c
> > @@ -33,8 +33,8 @@
> > #endif
> >
> > struct state {
> > - char *devname;
> > - char devnm[32]; /* to sync with mdstat info */
> > + char devname[MD_NAME_MAX + 8];
> > + char devnm[MD_NAME_MAX]; /* to sync with mdstat
> > info */ unsigned int utime;
> > int err;
> > char *spare_group;
> > @@ -45,7 +45,7 @@ struct state {
> > int devstate[MAX_DISKS];
> > dev_t devid[MAX_DISKS];
> > int percent;
> > - char parent_devnm[32]; /* For subarray, devnm of parent.
> > + char parent_devnm[MD_NAME_MAX]; /* For subarray, devnm of
> > parent.
> > * For others, ""
> > */
> > struct supertype *metadata;
> > @@ -187,13 +187,7 @@ int Monitor(struct mddev_dev *devlist,
> > continue;
> >
> > st = xcalloc(1, sizeof *st);
> > - if (mdlist->devname[0] == '/')
> > - st->devname =
> > xstrdup(mdlist->devname);
> > - else {
> > - st->devname =
> > xmalloc(8+strlen(mdlist->devname)+1);
> > - strcpy(strcpy(st->devname,
> > "/dev/md/"),
> > - mdlist->devname);
> > - }
> > + snprintf(st->devname, MD_NAME_MAX + 8,
> > "/dev/md/%s", basename(mdlist->devname));
>
> Hi Kinga,
>
> The above location is what the whole patch wants to fix. It is not
> worth to change other locations. IMHO the change set is a bit large
> than what it should be, just a suggestion maybe a following style
> change is enough (not test it),
>
> @@ -190,9 +190,13 @@ int Monitor(struct mddev_dev *devlist,
> if (mdlist->devname[0] == '/')
> st->devname =
> xstrdup(mdlist->devname); else {
> - st->devname =
> xmalloc(8+strlen(mdlist->devname)+1);
> - strcpy(strcpy(st->devname,
> "/dev/md/"),
> - mdlist->devname);
> + int _len;
> +
> + _len = 8 + strlen(mdlist->devname);
> + st->devname = xmalloc(_len + 1);
> + memset(st->devname, 0, _len + 1);
> + snprintf(st->devname, _len,
> "/dev/md/%s",
> + mdlist->devname);
> }
> if (!is_mddev(mdlist->devname))
> return 1;
>
> Thanks.
>
> Coly Li
>
>
>
> > if (!is_mddev(mdlist->devname))
> > return 1;
> > st->next = statelist;
> > @@ -216,7 +210,7 @@ int Monitor(struct mddev_dev *devlist,
> >
> > st = xcalloc(1, sizeof *st);
> > mdlist = conf_get_ident(dv->devname);
> > - st->devname = xstrdup(dv->devname);
> > + snprintf(st->devname, MD_NAME_MAX + 8,
> > "%s", dv->devname); st->next = statelist;
> > st->devnm[0] = 0;
> > st->percent = RESYNC_UNKNOWN;
> > @@ -299,7 +293,6 @@ int Monitor(struct mddev_dev *devlist,
> > for (stp = &statelist; (st = *stp) != NULL; ) {
> > if (st->from_auto && st->err > 5) {
> > *stp = st->next;
> > - free(st->devname);
> > free(st->spare_group);
> > free(st);
> > } else
> > @@ -552,7 +545,7 @@ static int check_array(struct state *st, struct
> > mdstat_ent *mdstat, goto disappeared;
> >
> > if (st->devnm[0] == 0)
> > - strcpy(st->devnm, fd2devnm(fd));
> > + snprintf(st->devnm, MD_NAME_MAX, "%s",
> > fd2devnm(fd));
> >
> > for (mse2 = mdstat; mse2; mse2 = mse2->next)
> > if (strcmp(mse2->devnm, st->devnm) == 0) {
> > @@ -682,7 +675,7 @@ static int check_array(struct state *st, struct
> > mdstat_ent *mdstat, strncmp(mse->metadata_version, "external:", 9)
> > == 0 && is_subarray(mse->metadata_version+9)) {
> > char *sl;
> > - strcpy(st->parent_devnm, mse->metadata_version +
> > 10);
> > + snprintf(st->parent_devnm, MD_NAME_MAX, "%s",
> > mse->metadata_version + 10); sl = strchr(st->parent_devnm, '/');
> > if (sl)
> > *sl = 0;
> > @@ -770,14 +763,13 @@ static int add_new_arrays(struct mdstat_ent
> > *mdstat, struct state **statelist, continue;
> > }
> >
> > - st->devname = xstrdup(name);
> > + snprintf(st->devname, MD_NAME_MAX + 8,
> > "%s", name); if ((fd = open(st->devname, O_RDONLY)) < 0 ||
> > md_get_array_info(fd, &array) < 0) {
> > /* no such array */
> > if (fd >= 0)
> > close(fd);
> > put_md_name(st->devname);
> > - free(st->devname);
> > if (st->metadata) {
> > st->metadata->ss->free_super(st->metadata);
> > free(st->metadata);
> > @@ -789,7 +781,7 @@ static int add_new_arrays(struct mdstat_ent
> > *mdstat, struct state **statelist, st->next = *statelist;
> > st->err = 1;
> > st->from_auto = 1;
> > - strcpy(st->devnm, mse->devnm);
> > + snprintf(st->devnm, MD_NAME_MAX, "%s",
> > mse->devnm); st->percent = RESYNC_UNKNOWN;
> > st->expected_spares = -1;
> > if (mse->metadata_version &&
> > @@ -797,8 +789,8 @@ static int add_new_arrays(struct mdstat_ent
> > *mdstat, struct state **statelist, "external:", 9) == 0 &&
> > is_subarray(mse->metadata_version+9)) {
> > char *sl;
> > - strcpy(st->parent_devnm,
> > - mse->metadata_version+10);
> > + snprintf(st->parent_devnm,
> > MD_NAME_MAX,
> > + "%s",
> > mse->metadata_version + 10); sl = strchr(st->parent_devnm, '/');
> > *sl = 0;
> > } else
> > --
> > 2.26.2
> >
>
Hi Coly,
Yes, fragment of code, which you selected, is enough to fix it. I have
changed also 32 to define, and fixed unsafe functions in this file to
improve code quality and avoid magic number. What's your opinion, is
this change worth to add?
Would you mind getting two patches, one with this specific fix, and
other one with another improvements in this file?
Regards,
Kinga
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] Monitor: use devname as char array instead of pointer
2022-07-05 12:05 ` Kinga Tanska
@ 2022-07-05 14:06 ` Coly Li
0 siblings, 0 replies; 4+ messages in thread
From: Coly Li @ 2022-07-05 14:06 UTC (permalink / raw)
To: Kinga Tanska; +Cc: linux-raid, jes
> 2022年7月5日 20:05，Kinga Tanska <kinga.tanska@linux.intel.com> 写道：
>
> On Sat, 25 Jun 2022 15:03:32 +0800
> Coly Li <colyli@suse.de> wrote:
>
>>> 2022年6月20日 22:06，Kinga Tanska <kinga.tanska@intel.com> 写道：
>>>
>>> Device name wasn't filled properly due to incorrect use of strcpy.
>>> Strcpy was used twice. Firstly to fill devname with "/dev/md/"
>>> and then to add chosen name. First strcpy result was overwritten by
>>> second one (as a result <device_name> instead of
>>> "/dev/md/<device_name>" was assigned). This commit changes this
>>> implementation to use snprintf and devname with fixed size. Also
>>> safer string functions are propagated.
>>>
>>> Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
>>> ---
>>> Monitor.c | 30 +++++++++++-------------------
>>> 1 file changed, 11 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/Monitor.c b/Monitor.c
>>> index 6ca1ebe5..04112791 100644
>>> --- a/Monitor.c
>>> +++ b/Monitor.c
>>> @@ -33,8 +33,8 @@
>>> #endif
>>>
>>> struct state {
>>> - char *devname;
>>> - char devnm[32]; /* to sync with mdstat info */
>>> + char devname[MD_NAME_MAX + 8];
>>> + char devnm[MD_NAME_MAX]; /* to sync with mdstat
>>> info */ unsigned int utime;
>>> int err;
>>> char *spare_group;
>>> @@ -45,7 +45,7 @@ struct state {
>>> int devstate[MAX_DISKS];
>>> dev_t devid[MAX_DISKS];
>>> int percent;
>>> - char parent_devnm[32]; /* For subarray, devnm of parent.
>>> + char parent_devnm[MD_NAME_MAX]; /* For subarray, devnm of
>>> parent.
>>> * For others, ""
>>> */
>>> struct supertype *metadata;
>>> @@ -187,13 +187,7 @@ int Monitor(struct mddev_dev *devlist,
>>> continue;
>>>
>>> st = xcalloc(1, sizeof *st);
>>> - if (mdlist->devname[0] == '/')
>>> - st->devname =
>>> xstrdup(mdlist->devname);
>>> - else {
>>> - st->devname =
>>> xmalloc(8+strlen(mdlist->devname)+1);
>>> - strcpy(strcpy(st->devname,
>>> "/dev/md/"),
>>> - mdlist->devname);
>>> - }
>>> + snprintf(st->devname, MD_NAME_MAX + 8,
>>> "/dev/md/%s", basename(mdlist->devname));
>>
>> Hi Kinga,
>>
>> The above location is what the whole patch wants to fix. It is not
>> worth to change other locations. IMHO the change set is a bit large
>> than what it should be, just a suggestion maybe a following style
>> change is enough (not test it),
>>
>> @@ -190,9 +190,13 @@ int Monitor(struct mddev_dev *devlist,
>> if (mdlist->devname[0] == '/')
>> st->devname =
>> xstrdup(mdlist->devname); else {
>> - st->devname =
>> xmalloc(8+strlen(mdlist->devname)+1);
>> - strcpy(strcpy(st->devname,
>> "/dev/md/"),
>> - mdlist->devname);
>> + int _len;
>> +
>> + _len = 8 + strlen(mdlist->devname);
>> + st->devname = xmalloc(_len + 1);
>> + memset(st->devname, 0, _len + 1);
>> + snprintf(st->devname, _len,
>> "/dev/md/%s",
>> + mdlist->devname);
>> }
>> if (!is_mddev(mdlist->devname))
>> return 1;
>>
>> Thanks.
>>
>> Coly Li
>>
>>
>>
>>> if (!is_mddev(mdlist->devname))
>>> return 1;
>>> st->next = statelist;
>>> @@ -216,7 +210,7 @@ int Monitor(struct mddev_dev *devlist,
>>>
>>> st = xcalloc(1, sizeof *st);
>>> mdlist = conf_get_ident(dv->devname);
>>> - st->devname = xstrdup(dv->devname);
>>> + snprintf(st->devname, MD_NAME_MAX + 8,
>>> "%s", dv->devname); st->next = statelist;
>>> st->devnm[0] = 0;
>>> st->percent = RESYNC_UNKNOWN;
>>> @@ -299,7 +293,6 @@ int Monitor(struct mddev_dev *devlist,
>>> for (stp = &statelist; (st = *stp) != NULL; ) {
>>> if (st->from_auto && st->err > 5) {
>>> *stp = st->next;
>>> - free(st->devname);
>>> free(st->spare_group);
>>> free(st);
>>> } else
>>> @@ -552,7 +545,7 @@ static int check_array(struct state *st, struct
>>> mdstat_ent *mdstat, goto disappeared;
>>>
>>> if (st->devnm[0] == 0)
>>> - strcpy(st->devnm, fd2devnm(fd));
>>> + snprintf(st->devnm, MD_NAME_MAX, "%s",
>>> fd2devnm(fd));
>>>
>>> for (mse2 = mdstat; mse2; mse2 = mse2->next)
>>> if (strcmp(mse2->devnm, st->devnm) == 0) {
>>> @@ -682,7 +675,7 @@ static int check_array(struct state *st, struct
>>> mdstat_ent *mdstat, strncmp(mse->metadata_version, "external:", 9)
>>> == 0 && is_subarray(mse->metadata_version+9)) {
>>> char *sl;
>>> - strcpy(st->parent_devnm, mse->metadata_version +
>>> 10);
>>> + snprintf(st->parent_devnm, MD_NAME_MAX, "%s",
>>> mse->metadata_version + 10); sl = strchr(st->parent_devnm, '/');
>>> if (sl)
>>> *sl = 0;
>>> @@ -770,14 +763,13 @@ static int add_new_arrays(struct mdstat_ent
>>> *mdstat, struct state **statelist, continue;
>>> }
>>>
>>> - st->devname = xstrdup(name);
>>> + snprintf(st->devname, MD_NAME_MAX + 8,
>>> "%s", name); if ((fd = open(st->devname, O_RDONLY)) < 0 ||
>>> md_get_array_info(fd, &array) < 0) {
>>> /* no such array */
>>> if (fd >= 0)
>>> close(fd);
>>> put_md_name(st->devname);
>>> - free(st->devname);
>>> if (st->metadata) {
>>> st->metadata->ss->free_super(st->metadata);
>>> free(st->metadata);
>>> @@ -789,7 +781,7 @@ static int add_new_arrays(struct mdstat_ent
>>> *mdstat, struct state **statelist, st->next = *statelist;
>>> st->err = 1;
>>> st->from_auto = 1;
>>> - strcpy(st->devnm, mse->devnm);
>>> + snprintf(st->devnm, MD_NAME_MAX, "%s",
>>> mse->devnm); st->percent = RESYNC_UNKNOWN;
>>> st->expected_spares = -1;
>>> if (mse->metadata_version &&
>>> @@ -797,8 +789,8 @@ static int add_new_arrays(struct mdstat_ent
>>> *mdstat, struct state **statelist, "external:", 9) == 0 &&
>>> is_subarray(mse->metadata_version+9)) {
>>> char *sl;
>>> - strcpy(st->parent_devnm,
>>> - mse->metadata_version+10);
>>> + snprintf(st->parent_devnm,
>>> MD_NAME_MAX,
>>> + "%s",
>>> mse->metadata_version + 10); sl = strchr(st->parent_devnm, '/');
>>> *sl = 0;
>>> } else
>>> --
>>> 2.26.2
>>>
>>
>
> Hi Coly,
>
> Yes, fragment of code, which you selected, is enough to fix it. I have
> changed also 32 to define, and fixed unsafe functions in this file to
> improve code quality and avoid magic number. What's your opinion, is
> this change worth to add?
> Would you mind getting two patches, one with this specific fix, and
> other one with another improvements in this file?
It will be cool, if the patch can be split into read-fix and code-clean-up parts.
So I can provide my feedback fast for the real fix part.
Thanks.
Coly Li
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-05 14:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-20 14:06 [PATCH v3] Monitor: use devname as char array instead of pointer Kinga Tanska
2022-06-25 7:03 ` Coly Li
2022-07-05 12:05 ` Kinga Tanska
2022-07-05 14:06 ` Coly Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox