> 2022年6月20日 22:06，Kinga Tanska <kinga.tanska@xxxxxxxxx> 写道：
>
> 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@xxxxxxxxx>
> ---
> 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
>
