From: Nikolay Sivov <nsivov@codeweavers.com>
Subject: Re: [PATCH v3 3/3] xmllite: Expand test for any unparsed data at end of XML.
Message-Id: <30300517-0b38-2b54-d950-32726cd6c362@codeweavers.com>
Date: Sat, 7 Dec 2019 01:18:46 +0300
In-Reply-To: <CAPX7N=6L9aUtLV+TNXJB1C1Buc3Ghv7CunKN9xJ7w4eiY7TZYg@mail.gmail.com>
References: <20191205195326.928106-1-whydoubt@gmail.com> <20191205195326.928106-3-whydoubt@gmail.com> <c570f623-7b2c-2fc5-0232-cc68a2bb6602@codeweavers.com> <CAPX7N=6L9aUtLV+TNXJB1C1Buc3Ghv7CunKN9xJ7w4eiY7TZYg@mail.gmail.com>

On 12/7/19 12:24 AM, Jeff Smith wrote:
> On Fri, Dec 6, 2019 at 11:16 AM Nikolay Sivov <nsivov@codeweavers.com> wrote:
>> On 12/5/19 10:53 PM, Jeff Smith wrote:
>>> @@ -2662,7 +2663,7 @@ static HRESULT reader_parse_nextnode(xmlreader *reader)
>>>                hr = reader_parse_misc(reader);
>>>                if (hr != S_FALSE) return hr;
>>>
>>> -            if (*reader_get_ptr(reader))
>>> +            if (buffer->cur*sizeof(WCHAR) < buffer->written)
>>>                {
>>>                    WARN("found garbage in the end of XML\n");
>>>                    return WC_E_SYNTAX;
> Hi Nikolay,
>
>> That means we don't have enough data,
> How do you figure that?
>
>> it's another change not backed by tests
> This fixes two tests, and does not break any others.
>
>> and potentially depending on current read-ahead buffer size/filled level.
> I'm pretty sure reader_parse_misc would have read at least one byte
> ahead, which is all that is required for this to trigger, though I
> could double-check that.
> However, to your point made in the patch 2 of the set about not
> exposing the buffer at this level, I will also consider this something
> that potentially needs to be handled elsewhere.
My point is that we should always hit this single invalid syntax/garbage 
at the end condition that we already have,
instead of doing fixups for specific node types.
>
> Regards,
> Jeff