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

From: Harald van Dijk <harald@gigawatt.nl>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>, dash@vger.kernel.org
Subject: Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line
Date: Wed, 4 Jan 2023 11:25:09 +0000	[thread overview]
Message-ID: <8ddcc8a3-9588-435a-9d5f-1edbcd121012@gigawatt.nl> (raw)
In-Reply-To: <Y7VMJGPXXziUrBio@gondor.apana.org.au>

On 04/01/2023 09:51, Herbert Xu wrote:
> On Tue, Jan 03, 2023 at 11:47:05AM +0000, Harald van Dijk wrote:
>>
>> 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.
> 
> You're right, it isn't possible with TDO.
> 
>> 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.
> 
> However, the synexpect in parsefname would seem to qualify:
> 
> cat <<- <newline>

You're right, that does show the problem, thanks.

Since the only case where special care is needed is when the problematic 
token was newline, it can be handled as simply as

   if (lasttoken != TNL)
     skipline();

I will do some testing to see if I am overlooking something, and follow 
up if I am.

>> 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.
> 
> I don't think this is such a big deal.  You get the same effect
> if you simply do input line buffering but only up to a certain
> length.  I tried one million instead of ten thousand with the
> reproducer and ksh93 prints out the error after only 65536
> characters.
> 
> In any case, we could actually fix this by buffering stderr and
> then making sh -v bypass the buffer and write directly to 2.
> 
> I tried it and it actually seems to work.  But all the extra flush
> calls bloat up the code a bit so it needs a bit more work before
> I'm happy to run with it.

Interesting approach. Error messages are known to always fit in the 
buffer, so you can probably be sure it won't be forced to be emitted 
early. It could work.

Cheers,
Harald van Dijk