On 11/3/21 5:08 AM, Tkaczyk, Mariusz wrote:
>> The switch to using NAME_MAX has some side effects I am a little
>> concerned about, however the code is also really tricky to get your head
>> around (not your fault).
>>
> My first idea was to rewrite it at all but there is more nits
> (like --name parameter and config). It is not a task for rc stage.
Agree lets look at this after 4.2
>>> @@ -188,35 +209,48 @@ int create_mddev(char *dev, char *name, int
>>> autof, int trustworthy,
>>> parts = autof >> 3;
>>> autof &= 7;
>>> - strcpy(chosen, "/dev/md/");
>>> - cname = chosen + strlen(chosen);
>>> + if (chosen_size <= strlen(dev_md_path) + cname_size) {
>>> + dprintf("Chosen buffer is to small.\n");
>>> + return -1;
>>> + }
>>
>> cname_size = NAME_MAX = 255
>>
>> Ie. if something calls create_mddev() with a chosen_size smaller than
>> 263, this check will fail, which seems rather arbitrary. It does look
>> like we always use a chosen_name[1024] which is silly wasteful, but
>> there much be a better solution to this. Maybe malloc() and return the
>> buffer instead of relying on those large stack frames?
>
> With malloc, I will need to add free() everywhere, I don't think
> that it is good option.
> I agree that this check can be removed, especially that now it is
> always called with size 1024.
I'd love to get rid of those 1024 byte arrays too.
>>> + strncpy(chosen, dev_md_path, chosen_size);
>>> + cname = chosen + strlen(dev_md_path);
>>> if (dev) {
>>> - if (strncmp(dev, "/dev/md/", 8) == 0) {
>>> - strcpy(cname, dev+8);
>>> - } else if (strncmp(dev, "/dev/", 5) == 0) {
>>> - char *e = dev + strlen(dev);
>>> + if (strncmp(dev, dev_md_path, 8) == 0)
>>> + strncpy(cname, dev+8, cname_size);
>>
>> sizeof(dev_md_path) instead of the hardcoded 8?
>>
>>> + else if (strncmp(dev, dev_md_path, 5) == 0) {
>>> + const char *e = dev + 5 + strnlen(dev + 5, NAME_MAX);
>>> +> while (e > dev && isdigit(e[-1]))
>>> e--;> if (e[0])
>>> num = strtoul(e, NULL, 10);
>>> - strcpy(cname, dev+5);
>>> + strncpy(cname, dev + 5, cname_size);
>>> cname[e-(dev+5)] = 0;
>>> +
>>> /* name *must* be mdXX or md_dXX in this context */
>>> if (num < 0 ||
>>> - (strcmp(cname, "md") != 0 && strcmp(cname, "md_d")
>>> != 0)) {
>>> + (strncmp(cname, "md", 2) != 0 &&
>>> + strncmp(cname, "md_d", 4) != 0)) {
>>> pr_err("%s is an invalid name for an md device.
>>> Try /dev/md/%s\n",
>>> dev, dev+5);
>>> return -1;
>>> }
>>> - if (strcmp(cname, "md") == 0)
>>> + if (strncmp(cname, "md", 2) == 0)
>>> use_mdp = 0;
>>> else
>>> use_mdp = 1;
>>> /* recreate name: /dev/md/0 or /dev/md/d0 */
>>> - sprintf(cname, "%s%d", use_mdp?"d":"", num);
>>> + snprintf(cname, cname_size, "%s%d",
>>> + use_mdp ? "d" : "", num);
>>> } else
>>> - strcpy(cname, dev);
>>> + strncpy(cname, dev, cname_size);
>>> + /*
>>> + * Force null termination for long names.
>>> + */
>>> + cname[cname_size - 1] = '\0';
>>> /* 'cname' must not contain a slash, and may not be
>>> * empty.
>>
>> My head started spinning by the time I got to here.
>>
>> The more I think about it, the more I think we should allocate an
>> appropriate buffer in here and return that, rather than play all those
>> bounds checking games.
>>
>> Thoughts?
>>
> We need to return mdfd too, so cannot return from here.
We can still return it in a pointer argument.
> First we need to determine how it should work.
> The code now is quite unpredictable but it is working for
> years. I just tried to fix static code analysis errors for
> now. My concerns are here:
> #mdadm -CR /dev/mdx --name=test ...
> #mdadm -CR name --name=test ...
> #mdadm -CR /dev/md/name --name=test ...
>
> We can define volume by *dev and *name (--name=) and it
> is not well defined how it should behave. I will need
> to start with determining it first and later adapt
> other stuff (assemble and incremental).
>
> So, it requires separate discussion and will
> be a release blocker.
Yes this is why reading the code made my head spin :)
Cheers,
Jes
