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

[Doug Anderson]

Hi,

On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
>
> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> 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?

-Doug