From: Jacek Caban <jacek@codeweavers.com>
Subject: Re: [PATCH] ieframe: Clear a being invalidated history entry.
Message-Id: <d152c21c-f0ee-2930-ecd3-556896e15488@codeweavers.com>
Date: Wed, 26 Jan 2022 12:57:47 +0100
In-Reply-To: <20220125214746.ce54665975e664aa68a45dd3@baikal.ru>
References: <20220124170445.3e7ba06816d83527e7277d77@baikal.ru> <0dc6e9f0-55c6-cfd0-4326-903b98f5d4ce@codeweavers.com> <20220125214746.ce54665975e664aa68a45dd3@baikal.ru>

On 1/25/22 19:47, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Jacek Caban <jacek@codeweavers.com> wrote:
>
>> On 1/24/22 15:04, Dmitry Timoshkov wrote:
>>> update_travellog() in order to clear forward history calls free_travellog_entry() to
>>> invalidate forward history entries, and when later an entry gets reused entry->stream
>>> contains a no longer valid pointer.
>>
>> How does it "get reused"? Note that log buffer is also initially not
>> zero-initialized and generally depends on proper bounds checks.
>> update_travellog() decrements length when it clears forward history,
>> which should prevent us from treating those entries as valid.
> Probably "gets reused" is a wrong term. What I observe here is that once
> update_travellog() truncates the log, and position in the history is equal
> to the length, next call to go_forward() will crash because bounds check
> 'if (position >= length) return E_FAIL;' doesn't prevent referencing a no
> longer valid history entry. Does that explain what is going on?


Okay, so it's navigate_history() where the problem happens. It uses NULL 
check and the situation can only happen if forward history was cleared. 
Bound checks prevent from using an entry that was never initialized, so 
that case is fine. The patch looks good then.


Thanks,

Jacek