Fix memory issues in ScrollView::MessageReceiver

[p12tic]

This PR fixes memory issues that caused #3869. The first commit fixes the actual bug (unique_ptr::release() should be used instead of unique_ptr::get()). The second fixes the underlying problem of using plain pointers that caused the bug in the first place.

Fixes #3869.

Supersedes #3870

FYI @rmast @stweil.

[p12tic]

@rmast Could you please check whether this PR fixes the issue you were seeing. I didn't try to reproduce the problem, only diagnosed the problem by looking into source code.

[stweil]

I still get an error when running with valgrind:

$ valgrind bin/debug/clang/tesseract -c invert_threshold=0.9 --dpi 300 -l Latin -c textord_debug_tabfind=1 -c textord_debug_bugs=1 -c tessedit_reject_block_percent=1 -c tessedit_reject_row_percent=1 -c debug_noise_removal=1 https://user-images.githubusercontent.com/3341558/179373903-ef6cc246-f4e5-4633-a762-ded4dd22708f.jpg output82
==1212620== Memcheck, a memory error detector
==1212620== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1212620== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==1212620== Command: bin/debug/clang/tesseract -c invert_threshold=0.9 --dpi 300 -l tessdata_fast/script/Latin -c textord_debug_tabfind=1 -c textord_debug_bugs=1 -c tessedit_reject_block_percent=1 -c tessedit_reject_row_percent=1 -c debug_noise_removal=1 https://user-images.githubusercontent.com/3341558/179373903-ef6cc246-f4e5-4633-a762-ded4dd22708f.jpg output81
==1212620== 
Vertical skew vector=(7,13173)
Starting sh -c "trap 'kill %1' 0 1 2 ; java -Xms1024m -Xmx2048m -jar ./ScrollView.jar & wait"
ScrollView: Waiting for server...
Socket started on port 8461
Client connected
Click at (855, 2749)
Click at (855, 2749)
==1212620== Thread 2:
==1212620== Invalid read of size 8
==1212620==    at 0x54AF90: tesseract::SVEvent::~SVEvent() (scrollview.h:70)
==1212620==    by 0x6013EA: std::default_delete<tesseract::SVEvent>::operator()(tesseract::SVEvent*) const (unique_ptr.h:85)
==1212620==    by 0x60050C: std::unique_ptr<tesseract::SVEvent, std::default_delete<tesseract::SVEvent> >::~unique_ptr() (unique_ptr.h:361)
==1212620==    by 0x5FD91F: tesseract::ScrollView::MessageReceiver() (scrollview.cpp:184)
==1212620==    by 0x604056: void std::__invoke_impl<void, void (*)()>(std::__invoke_other, void (*&&)()) (invoke.h:60)
==1212620==    by 0x603FEC: std::__invoke_result<void (*)()>::type std::__invoke<void (*)()>(void (*&&)()) (invoke.h:95)
==1212620==    by 0x603FC4: void std::thread::_Invoker<std::tuple<void (*)()> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) (thread:264)
==1212620==    by 0x603F94: std::thread::_Invoker<std::tuple<void (*)()> >::operator()() (thread:271)
==1212620==    by 0x603EED: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)()> > >::_M_run() (thread:215)
==1212620==    by 0x50D3ECF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==1212620==    by 0x4FEBEA6: start_thread (pthread_create.c:477)
==1212620==    by 0x52E9DEE: clone (clone.S:95)
==1212620==  Address 0xc15e4a0 is 16 bytes inside a block of size 48 free'd

[stweil]

With this additional change Tesseract no longer crashes:

diff --git a/src/viewer/scrollview.cpp b/src/viewer/scrollview.cpp
index 087dabe8..34d339f7 100644
--- a/src/viewer/scrollview.cpp
+++ b/src/viewer/scrollview.cpp
@@ -347,7 +347,7 @@ void ScrollView::StartEventHandler() {
       if (new_event->type == SVET_DESTROY) {
         // Signal the destructor that it is safe to terminate.
         event_handler_ended_ = true;
-        delete new_event; // Delete the pointer after it has been processed.
+        //delete new_event; // Delete the pointer after it has been processed.
         return;
       }
       delete new_event; // Delete the pointer after it has been processed.

I still don't know whether memory leaks remain.

[rmast]

I’m now running a build starting with the 2 commits from Povilas and adapted with two changes from Stefan. (One from this thread, one from a new commit on the main)

[stweil]

I still don't know whether memory leaks remain.

There remain two direct leaks (48 + 16 bytes) and an indirect leak (4 bytes). They are shown when I build tesseract with sanitizers (./configure --disable-shared --enable-debug CXX=clang++-11 'CXXFLAGS=-D_GLIBCXX_DEBUG -Wall -g -O0 -fsanitize=address,undefined -fstack-protector-strong').

[p12tic]

I still get an error when running with valgrind

@stweil I couldn't reproduce the problem and looking into code this crash seems to be something different than what the initial two commits fix.

Anyway, I have converted event_table_ to std::unique_ptr too. This should fix any memory handling bugs in that area.

[rmast]

This new run, with only Povilas’ branch with 3 force pushed commits did complete without a core dump. tesseract -c invert_threshold=0.9 --dpi 300 -l Latin -c textord_debug_tabfind=1 -c textord_debug_bugs=1 -c debug_noise_removal=1 -c textord_show_final_rows=1 -c textord_show_final_blobs=1 -c textord_show_initial_rows=1 -c textord_debug_blob=1 -c textord_oldbl_debug=1 -c textord_debug_baselines=1 -c textord_test_mode=1 -c classify_debug_level=1 -c dawg_debug_level=1 -c wordrec_debug_level=1 -c segsearch_debug_level=1 -c wordrec_display_segmentations=1 -c bidi_debug=1 -c debug_noise_removal=1 -c paragraph_debug_level=1 175789293-f39ddfdb-6f3e-4598-8d16-80a1f4a88b36.jpg output90

[p12tic]

Thanks @rmast for testing.

[stweil]

My test also passes without problems, and there remains no memory leak.

Thank you very much for this contribution!

[amitdo]

Nice work, thanks.