new valgrind errors in riched20

[Dan Kegel]

Seem to be four or so new valgrind warnings in riched20 today,
probably due to Dylan's changes (though my cat may have been
sufing to friskies.com and affected the results, who knows):
http://kegel.com/wine/valgrind/logs-2008-06-27/vg-riched20_editor-diff.txt

+ Invalid read of size 4
+    at  ME_AddUndoItem (undo.c:59)
+    by  ME_SetParaFormat (para.c:362)
+    by  ME_PlayUndoItem (undo.c:288)
+    by  ME_Undo (undo.c:360)
+    by  RichEditWndProc_common (editor.c:2067)
+    by  RichEditWndProcA (editor.c:3386)
+    by  ??? (library.h:163)
+    by  call_window_proc (winproc.c:457)
+    by  WINPROC_call_window (winproc.c:2207)
+    by  call_window_proc (message.c:1639)
+    by  send_message (message.c:2463)
+    by  SendMessageA (message.c:2608)
+    by  test_EM_SETUNDOLIMIT (editor.c:2238)
+    by  func_editor (editor.c:4581)
+    by  run_test (test.h:449)
+    by  main (test.h:498)

...

+ Conditional jump or move depends on uninitialised value(s)
+    at  test_EM_AUTOURLDETECT (editor.c:1719)
+    by  func_editor (editor.c:4600)
+    by  run_test (test.h:449)
+    by  main (test.h:498)
+  Uninitialised value was created by a client request
+    at  mark_block_uninitialized (heap.c:164)
+    by  RtlAllocateHeap (heap.c:1239)
+    by  heap_alloc (editor.h:28)
+    by  RichEditWndProc_common (editor.c:2573)
+    by  RichEditWndProcA (editor.c:3386)
+    by  ??? (library.h:163)
+    by  call_window_proc (winproc.c:457)
+    by  WINPROC_call_window (winproc.c:2207)
+    by  call_window_proc (message.c:1639)
+    by  send_message (message.c:2463)
+    by  SendMessageA (message.c:2608)
+    by  test_EM_AUTOURLDETECT (editor.c:1705)
+    by  func_editor (editor.c:4600)
+    by  run_test (test.h:449)
+    by  main (test.h:498)

...

+ Invalid write of size 4
+    at  ME_AddUndoItem (undo.c:62)
+    by  ME_InternalDeleteText (caret.c:322)
+    by  ME_PlayUndoItem (undo.c:308)
+    by  ME_Undo (undo.c:360)
+    by  RichEditWndProc_common (editor.c:2067)
+    by  RichEditWndProcW (editor.c:3381)
+    by  ??? (library.h:163)
+    by  call_window_proc (winproc.c:457)
+    by  WINPROC_CallProcAtoW (winproc.c:1011)
+    by  WINPROC_call_window (winproc.c:2209)
+    by  call_window_proc (message.c:1639)
+    by  send_message (message.c:2463)
+    by  SendMessageA (message.c:2608)
+    by  test_undo_coalescing (editor.c:4398)
+    by  func_editor (editor.c:4602)
+    by  run_test (test.h:449)
+    by  main (test.h:498)

...

+ Conditional jump or move depends on uninitialised value(s)
+    at  ME_AddUndoItem (undo.c:59)
+    by  ME_InternalDeleteText (caret.c:322)
+    by  ME_PlayUndoItem (undo.c:308)
+    by  ME_Undo (undo.c:360)
+    by  RichEditWndProc_common (editor.c:2067)
+    by  RichEditWndProcW (editor.c:3381)
+    by  ??? (library.h:163)
+    by  call_window_proc (winproc.c:457)
+    by  WINPROC_CallProcAtoW (winproc.c:1011)
+    by  WINPROC_call_window (winproc.c:2209)
+    by  call_window_proc (message.c:1639)
+    by  send_message (message.c:2463)
+    by  SendMessageA (message.c:2608)
+    by  test_undo_coalescing (editor.c:4398)
+    by  func_editor (editor.c:4602)
+    by  run_test (test.h:449)
+    by  main (test.h:498)
+  Uninitialised value was created by a client request
+    at  mark_block_uninitialized (heap.c:164)
+    by  RtlAllocateHeap (heap.c:1239)
+    by  heap_alloc (editor.h:28)
+    by  ME_MakeStringN (string.c:46)
+    by  ME_InternalDeleteText (caret.c:326)
+    by  ME_PlayUndoItem (undo.c:308)
+    by  ME_Undo (undo.c:360)
+    by  RichEditWndProc_common (editor.c:2067)
+    by  RichEditWndProcW (editor.c:3381)
+    by  ??? (library.h:163)
+    by  call_window_proc (winproc.c:457)
+    by  WINPROC_CallProcAtoW (winproc.c:1011)
+    by  WINPROC_call_window (winproc.c:2209)
+    by  call_window_proc (message.c:1639)
+    by  send_message (message.c:2463)
+    by  SendMessageA (message.c:2608)
+    by  test_undo_coalescing (editor.c:4398)
+    by  func_editor (editor.c:4602)
+    by  run_test (test.h:449)
+    by  main (test.h:498)

[Dylan Smith]

The error was a memory access of a freed object.  In ME_AddUndoItem I
checked the top of the undo stack to end a coalescing undo transaction,
assuming that this should be either a valid undo item, or NULL, instead
it was already freed.

The undo item being added was actually being added to the redo stack by
ME_Undo, and before this was done the top of the undo stack was
destroyed by not set to NULL, thus causing the valgrind error.

I fixed this in two places. First of all I moved my code to
conditionally turn a coalescing end transaction into an actual end
transaction, since it doesn't need to be done when adding to the redo
stack.  Second of all, I made sure the undo and redo stack are in
valid states for ME_Undo and ME_Redo before calling ME_AddUndoItem or
ME_PlayItem since I could see someone else making the same assumption.
This should fix the error and make it harder for a regression to occur.

Could you verify that Valgrind tests pass with the patch I attached.

I'll try to get started on using Valgrind, but for now I don't have it set
up.

On Fri, Jun 27, 2008 at 11:08 PM, Dan Kegel  wrote:
> Seem to be four or so new valgrind warnings in riched20 today,
> probably due to Dylan's changes (though my cat may have been
> sufing to friskies.com and affected the results, who knows):
> http://kegel.com/wine/valgrind/logs-2008-06-27/vg-riched20_editor-diff.txt
> - Invalid read of size 4
> - at  ME_AddUndoItem (undo.c:59)
> - by  ME_SetParaFormat (para.c:362)
> - by  ME_PlayUndoItem (undo.c:288)
> - by  ME_Undo (undo.c:360)
> - by  RichEditWndProc_common (editor.c:2067)
> - by  RichEditWndProcA (editor.c:3386)
> - by  ??? (library.h:163)
> - by  call_window_proc (winproc.c:457)
> - by  WINPROC_call_window (winproc.c:2207)
> - by  call_window_proc (message.c:1639)
> - by  send_message (message.c:2463)
> - by  SendMessageA (message.c:2608)
> - by  test_EM_SETUNDOLIMIT (editor.c:2238)
> - by  func_editor (editor.c:4581)
> - by  run_test (test.h:449)
> - by  main (test.h:498)
>
> ...
> - Conditional jump or move depends on uninitialised value(s)
> - at  test_EM_AUTOURLDETECT (editor.c:1719)
> - by  func_editor (editor.c:4600)
> - by  run_test (test.h:449)
> - by  main (test.h:498)
> - Uninitialised value was created by a client request
> - at  mark_block_uninitialized (heap.c:164)
> - by  RtlAllocateHeap (heap.c:1239)
> - by  heap_alloc (editor.h:28)
> - by  RichEditWndProc_common (editor.c:2573)
> - by  RichEditWndProcA (editor.c:3386)
> - by  ??? (library.h:163)
> - by  call_window_proc (winproc.c:457)
> - by  WINPROC_call_window (winproc.c:2207)
> - by  call_window_proc (message.c:1639)
> - by  send_message (message.c:2463)
> - by  SendMessageA (message.c:2608)
> - by  test_EM_AUTOURLDETECT (editor.c:1705)
> - by  func_editor (editor.c:4600)
> - by  run_test (test.h:449)
> - by  main (test.h:498)
>
> ...
> - Invalid write of size 4
> - at  ME_AddUndoItem (undo.c:62)
> - by  ME_InternalDeleteText (caret.c:322)
> - by  ME_PlayUndoItem (undo.c:308)
> - by  ME_Undo (undo.c:360)
> - by  RichEditWndProc_common (editor.c:2067)
> - by  RichEditWndProcW (editor.c:3381)
> - by  ??? (library.h:163)
> - by  call_window_proc (winproc.c:457)
> - by  WINPROC_CallProcAtoW (winproc.c:1011)
> - by  WINPROC_call_window (winproc.c:2209)
> - by  call_window_proc (message.c:1639)
> - by  send_message (message.c:2463)
> - by  SendMessageA (message.c:2608)
> - by  test_undo_coalescing (editor.c:4398)
> - by  func_editor (editor.c:4602)
> - by  run_test (test.h:449)
> - by  main (test.h:498)
>
> ...
> - Conditional jump or move depends on uninitialised value(s)
> - at  ME_AddUndoItem (undo.c:59)
> - by  ME_InternalDeleteText (caret.c:322)
> - by  ME_PlayUndoItem (undo.c:308)
> - by  ME_Undo (undo.c:360)
> - by  RichEditWndProc_common (editor.c:2067)
> - by  RichEditWndProcW (editor.c:3381)
> - by  ??? (library.h:163)
> - by  call_window_proc (winproc.c:457)
> - by  WINPROC_CallProcAtoW (winproc.c:1011)
> - by  WINPROC_call_window (winproc.c:2209)
> - by  call_window_proc (message.c:1639)
> - by  send_message (message.c:2463)
> - by  SendMessageA (message.c:2608)
> - by  test_undo_coalescing (editor.c:4398)
> - by  func_editor (editor.c:4602)
> - by  run_test (test.h:449)
> - by  main (test.h:498)
> - Uninitialised value was created by a client request
> - at  mark_block_uninitialized (heap.c:164)
> - by  RtlAllocateHeap (heap.c:1239)
> - by  heap_alloc (editor.h:28)
> - by  ME_MakeStringN (string.c:46)
> - by  ME_InternalDeleteText (caret.c:326)
> - by  ME_PlayUndoItem (undo.c:308)
> - by  ME_Undo (undo.c:360)
> - by  RichEditWndProc_common (editor.c:2067)
> - by  RichEditWndProcW (editor.c:3381)
> - by  ??? (library.h:163)
> - by  call_window_proc (winproc.c:457)
> - by  WINPROC_CallProcAtoW (winproc.c:1011)
> - by  WINPROC_call_window (winproc.c:2209)
> - by  call_window_proc (message.c:1639)
> - by  send_message (message.c:2463)
> - by  SendMessageA (message.c:2608)
> - by  test_undo_coalescing (editor.c:4398)
> - by  func_editor (editor.c:4602)
> - by  run_test (test.h:449)
> - by  main (test.h:498)