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.
This is an alternative patch that fixes a crash while navigating html help file that I have here.
Signed-off-by: Dmitry Timoshkov dmitry@baikal.ru --- dlls/ieframe/dochost.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/ieframe/dochost.c b/dlls/ieframe/dochost.c index 042417b5b17..1ba31c552ed 100644 --- a/dlls/ieframe/dochost.c +++ b/dlls/ieframe/dochost.c @@ -384,9 +384,12 @@ static LRESULT WINAPI doc_view_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM l
static void free_travellog_entry(travellog_entry_t *entry) { - if(entry->stream) + if(entry->stream) { IStream_Release(entry->stream); + entry->stream = NULL; + } heap_free(entry->url); + entry->url = NULL; }
static IStream *get_travellog_stream(DocHost *This)
Hi Dmitry,
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.
Thanks,
Jacek
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?
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
-
Dmitry Timoshkov -
Jacek Caban