[PATCH] ieframe: Make sure that history initial state is fully initialized.
In addition to 41cfc86d8dd65c71a7c81b826d02ac0a99050d6a. This fixes another crash when browsing help file that I have here. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/ieframe/dochost.c | 4 ++-- dlls/ieframe/ieframe.h | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/dlls/ieframe/dochost.c b/dlls/ieframe/dochost.c index 7717f786d52..4e1e1d82fa1 100644 --- a/dlls/ieframe/dochost.c +++ b/dlls/ieframe/dochost.c @@ -436,7 +436,7 @@ static void update_travellog(DocHost *This) } if(!This->travellog.log) { - This->travellog.log = heap_alloc(4 * sizeof(*This->travellog.log)); + This->travellog.log = heap_alloc_zero(4 * sizeof(*This->travellog.log)); if(!This->travellog.log) return; @@ -444,7 +444,7 @@ static void update_travellog(DocHost *This) }else if(This->travellog.size < This->travellog.position+1) { travellog_entry_t *new_travellog; - new_travellog = heap_realloc(This->travellog.log, This->travellog.size*2*sizeof(*This->travellog.log)); + new_travellog = heap_realloc_zero(This->travellog.log, This->travellog.size*2*sizeof(*This->travellog.log)); if(!new_travellog) return; diff --git a/dlls/ieframe/ieframe.h b/dlls/ieframe/ieframe.h index 633906a70ca..1bae1f6781d 100644 --- a/dlls/ieframe/ieframe.h +++ b/dlls/ieframe/ieframe.h @@ -340,6 +340,11 @@ static inline void unlock_module(void) { InterlockedDecrement(&module_ref); } +static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, SIZE_T len) +{ + return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, len); +} + static inline LPWSTR heap_strdupW(LPCWSTR str) { LPWSTR ret = NULL; -- 2.35.3
Hi Dmitry, On 4/25/22 13:50, Dmitry Timoshkov wrote:
diff --git a/dlls/ieframe/ieframe.h b/dlls/ieframe/ieframe.h index 633906a70ca..1bae1f6781d 100644 --- a/dlls/ieframe/ieframe.h +++ b/dlls/ieframe/ieframe.h @@ -340,6 +340,11 @@ static inline void unlock_module(void) { InterlockedDecrement(&module_ref); }
+static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, SIZE_T len) +{ + return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, len); +}
I think it would be better to use CRT memory allocators in ieframe instead of heap functions directly. Once we do that, there is no CRT counterpart to zeroing realloc. In this case, it seems to me that we'd want to initialize the entry when it becomes accessible, not necessarily on allocation. Thanks, Jacek
Hi Jacek, Jacek Caban <jacek(a)codeweavers.com> wrote:
On 4/25/22 13:50, Dmitry Timoshkov wrote:
diff --git a/dlls/ieframe/ieframe.h b/dlls/ieframe/ieframe.h index 633906a70ca..1bae1f6781d 100644 --- a/dlls/ieframe/ieframe.h +++ b/dlls/ieframe/ieframe.h @@ -340,6 +340,11 @@ static inline void unlock_module(void) { InterlockedDecrement(&module_ref); }
+static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, SIZE_T len) +{ + return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, len); +}
I think it would be better to use CRT memory allocators in ieframe instead of heap functions directly. Once we do that, there is no CRT counterpart to zeroing realloc.
I agree that moving to using CRT is the way to go, however that would probably make the fix more intrusive than necessary, so I decided to create a simpler patch.
In this case, it seems to me that we'd want to initialize the entry when it becomes accessible, not necessarily on allocation.
I'm not sure I understand what you mean by entry being made accessible, could you please clarify? As far I can see from the ieframe code inspection IWebBrowser::GoForward()/IWebBrowser::GoBack() directly call helpers that rely on log entries being already intialized, and the only place where a log entry gets intialized is dochost.c,update_travellog(). -- Dmitry.
On 4/26/22 17:06, Dmitry Timoshkov wrote:
Hi Jacek,
Jacek Caban <jacek(a)codeweavers.com> wrote:
On 4/25/22 13:50, Dmitry Timoshkov wrote:
diff --git a/dlls/ieframe/ieframe.h b/dlls/ieframe/ieframe.h index 633906a70ca..1bae1f6781d 100644 --- a/dlls/ieframe/ieframe.h +++ b/dlls/ieframe/ieframe.h @@ -340,6 +340,11 @@ static inline void unlock_module(void) { InterlockedDecrement(&module_ref); }
+static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, SIZE_T len) +{ + return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, len); +}
I think it would be better to use CRT memory allocators in ieframe instead of heap functions directly. Once we do that, there is no CRT counterpart to zeroing realloc. I agree that moving to using CRT is the way to go, however that would probably make the fix more intrusive than necessary, so I decided to create a simpler patch.
Sure, it's fine if you don't do the conversion, but please don't make it harder by introducing an unneeded function that we will need to get rid anyway.
In this case, it seems to me that we'd want to initialize the entry when it becomes accessible, not necessarily on allocation. I'm not sure I understand what you mean by entry being made accessible, could you please clarify? As far I can see from the ieframe code inspection IWebBrowser::GoForward()/IWebBrowser::GoBack() directly call helpers that rely on log entries being already intialized, and the only place where a log entry gets intialized is dochost.c,update_travellog().
I mean to initialize it when we start considering the new entry to be valid. It seems that in this case, it's loading_pos == -1 handling in update_travellog. We may initialize just the new entry there. Thanks, Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
In this case, it seems to me that we'd want to initialize the entry when it becomes accessible, not necessarily on allocation. I'm not sure I understand what you mean by entry being made accessible, could you please clarify? As far I can see from the ieframe code inspection IWebBrowser::GoForward()/IWebBrowser::GoBack() directly call helpers that rely on log entries being already intialized, and the only place where a log entry gets intialized is dochost.c,update_travellog().
I mean to initialize it when we start considering the new entry to be valid. It seems that in this case, it's loading_pos == -1 handling in update_travellog. We may initialize just the new entry there.
Thank you very much for the detailed analysis, initializing history log entry in the loading_pos == -1 case also does fix the crash. I sent new version of the patch. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Jacek Caban