> 2022年7月5日 20:05，Kinga Tanska <kinga.tanska@xxxxxxxxxxxxxxx> 写道：
>
> On Sat, 25 Jun 2022 15:03:32 +0800
> Coly Li <colyli@xxxxxxx> wrote:
>
>>> 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
>>>
>>
>
> 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
