Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>>
>> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@chromium.org> wrote:
>> >
>> > Hi,
>> >
>> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>> > >
>> > > +static void
>> > > +match_identity(const struct detailed_timing *timing, void *data)
>> > > +{
>> > > +       struct drm_edid_match_closure *closure = data;
>> > > +       unsigned int i;
>> > > +       const char *name = closure->ident->name;
>> > > +       unsigned int name_len = strlen(name);
>> > > +       const char *desc = timing->data.other_data.data.str.str;
>> > > +       unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
>> > > +
>> > > +       if (name_len > desc_len ||
>> > > +           !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
>> > > +             is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
>> > > +               return;
>> > > +
>> > > +       if (strncmp(name, desc, name_len))
>> > > +               return;
>> > > +
>> > > +       /* Allow trailing white spaces and \0. */
>> > > +       for (i = name_len; i < desc_len; i++) {
>> > > +               if (desc[i] == '\n')
>> > > +                       break;
>> > > +               if (!isspace(desc[i]) && !desc[i])
>> > > +                       return;
>> > > +       }
>> >
>> > If my code analysis is correct, I think you'll reject the case where:
>> >
>> > name = "foo"
>> > desc[13] = "foo \0zzzzzzzz"
>> >
>> > ...but you'll accept these cases:
>> >
>> > desc[13] = "foo \nzzzzzzzz"
>> > desc[13] = "foo \0\0\0\0\0\0\0\0\0"
>> >
>> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
>>
>> I'm also not sure about \0... based on
>> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
>> they use \n as terminator. Maybe we should also reject \0 before\n?
>> Since it's not printable.
>
> Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
> I guess in that case I'd prefer simply removing the code to handle
> '\0' instead of treating it like space until we see some actual need
> for it. So just get rid of the "!desc[i]" case?

The spec text, similar for both EDID_DETAIL_MONITOR_NAME and
EDID_DETAIL_MONITOR_STRING:

	Up to 13 alphanumeric characters (using ASCII codes) may be used
	to define the model name of the display product. The data shall
	be sequenced such that the 1st byte (ASCII code) = the 1st
	character, the 2nd byte (ASCII code) = the 2nd character,
	etc. If there are less than 13 characters in the string, then
	terminate the display product name string with ASCII code ‘0Ah’
	(line feed) and pad the unused bytes in the field with ASCII
	code ‘20h’ (space).

In theory, only checking for '\n' for termination should be enough, and
this is what drm_edid_get_monitor_name() does. If there's a space
*before* that, it should be considered part of the name, and not
ignored. (So my suggestion in reply to the previous version is wrong.)

However, since the match name uses NUL termination, maybe we should
ignore NULs *before* '\n'? Like so:

for (i = name_len; i < desc_len; i++) {
        if (desc[i] == '\n')
                break;
        if (!desc[i])
                return;
}


BR,
Jani.


>
> -Doug

-- 
Jani Nikula, Intel