Re: [PATCH v3] libcpp: Implement -Wbidi-chars for CVE-2021-42574 [PR103026]

[Marek Polacek <polacek@redhat.com>]

On Tue, Nov 16, 2021 at 09:28:21PM -0500, David Malcolm wrote:
> On Tue, 2021-11-16 at 19:37 -0500, Marek Polacek wrote:
> > Sorry for a dumb question, but is this what you have in mind?
> > 
> > /* LRE
> >    PDF */
> > /* FSI
> >    PDI */
> > and check that we warn for these?
> 
> I mean something like the following multiline comments in which lines
> within them at the start, middle and end have unpaired constructs
> within a given line:
> 
> 
> /* RLI
>  *
>  */
> 
> /*
>  * RLI
>  */
> 
> /*
>  *  
>  * RLI */
> 
> and that we should warn for each case at the line containing the
> unpaired control character.
> 
> (the above lines don't have the actual chars, just "RLI")
> 
> Mostly this is just me trying to think about it from a black-box
> testing perspective, or in case we ever touch this code in the future
> (perhaps it's obviously correct by inspection of the implementation
> now, but let's have regression tests for these cases).
> 
> Sorry to add more work, but here's an idea for another test case:
> multiple comments on one line:
> 
>   /* RLI */  /* PDF */
> 
> where the closure of a comment should trigger closing a "context", so
> we should complain about the above.

No problem, I've added these.
 
> > 
> > > > > > @@ -1505,13 +1855,17 @@ lex_identifier (cpp_reader *pfile,
> > > > > > const uchar *base, bool starts_ucn,
> > > > > >      {
> > > > > >        /* Slower version for identifiers containing UCNs
> > > > > >          or extended chars (including $).  */
> > > > > > -      do {
> > > > > > -       while (ISIDNUM (*pfile->buffer->cur))
> > > > > > -         {
> > > > > > -           NORMALIZE_STATE_UPDATE_IDNUM (nst, *pfile->buffer-
> > > > > > >cur);
> > > > > > -           pfile->buffer->cur++;
> > > > > > -         }
> > > > > > -      } while (forms_identifier_p (pfile, false, nst));
> > > > > > +      do
> > > > > > +       {
> > > > > > +         while (ISIDNUM (*pfile->buffer->cur))
> > > > > > +           {
> > > > > > +             NORMALIZE_STATE_UPDATE_IDNUM (nst, *pfile-
> > > > > > >buffer->cur);
> > > > > > +             pfile->buffer->cur++;
> > > > > > +           }
> > > > > > +       }
> > > > > > +      while (forms_identifier_p (pfile, false, nst));
> > > > > 
> > > > > Is the above purely a whitespace change?
> > > > 
> > > > Yes.
> > > 
> > > If I'm reading things correctly, these lines in the existing code
> > > were
> > > correctly indented, so is there a purpose to this change?  If not,
> > > please can you remove this change from the patch (to minimize the
> > > change to the history).
> > 
> > I dropped that change then.  Sometimes it's hard to resist fixing
> > formatting.  ;)
> 
> Thanks.  But I don't think the existing formatting in the code *was*
> broken; I thought the patch was taking correct formatting and breaking
> it (hence my objection to a whitespace change).  If I misread this,
> sorry.

I think it was, we're supposed to format do-while as

  do
    {
    }
  while (...);

but it's obviously not a big deal.

> Hopefully the above makes sense and is constructive; let me know when
> you push your patch so that I can work on my followup.

Pushed now.  Thanks!

Marek