Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Harald van Dijk <harald@gigawatt.nl>]

On 03/01/2023 01:53, Herbert Xu wrote:
> наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
>>
>> synerror(const char *msg)
>> {
>>         errlinno = plinno;
>> +
>> +       /* If we see a syntax error in a command, read the rest of the
>> +        * line now before reporting the error. This ensures we get error
>> +        * reporting that does not depend on buffering details. */
>> +       skipline();
> 
> This is broken.  What if we already read a newline just before
> the syntax error (e.g., synexpect(TDO))?

In order for this to be a problem, we need something where the newline 
itself triggers a syntax error. For synexpect(TDO), is that possible? In 
all cases where synexpect(TDO) is called, a newline is permitted, and it 
is the token after the newline that the error will be reported on, no? 
In that situation, reading the rest of the line is correct.

I'm not going to rule out that there is a potential for problems, but if 
there is, it's not the situation you say.

> This needs to be dealt with in the input layer.

Handling it in the input layer after the error has already been reported 
means we get inconsistent error handling when running with dash -isv. 
The error may, depending on whether the rest of the line was in the 
buffer already, be reported either in the middle of the input line, or 
on the next line.

Just try changing sh -i to sh -iv in the reproducer and seeing what what 
happens with your patch.

Cheers,
Harald van Dijk