From 4f831ff48967c6a8b7f927051bda1b432e105772 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 18 Jul 2022 18:04:29 +0300 Subject: [PATCH 1/3] viewer: Fix double free caused by ScrollView::MessageReceiver waiting_for_events takes ownership of the passed event which is later deleted. Since we use unique_ptr::get() to acquire the pointer, we cause double free: one free happens in the code path where the event from waiting_for_events goes and the other free happens in unique_ptr destructor. The fix is to move ownership out of unique_ptr by unique_ptr::release(). Fixes: https://github.com/tesseract-ocr/tesseract/issues/3869 Fixes: 37b33749da56f5762346e16d0cc843460caacdb5 --- src/viewer/scrollview.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/viewer/scrollview.cpp b/src/viewer/scrollview.cpp index 7a9afebb48..3e3517b638 100644 --- a/src/viewer/scrollview.cpp +++ b/src/viewer/scrollview.cpp @@ -158,13 +158,13 @@ void ScrollView::MessageReceiver() { SVET_ANY); waiting_for_events_mu->lock(); if (waiting_for_events.count(awaiting_list) > 0) { - waiting_for_events[awaiting_list].second = cur.get(); + waiting_for_events[awaiting_list].second = cur.release(); waiting_for_events[awaiting_list].first->Signal(); } else if (waiting_for_events.count(awaiting_list_any) > 0) { - waiting_for_events[awaiting_list_any].second = cur.get(); + waiting_for_events[awaiting_list_any].second = cur.release(); waiting_for_events[awaiting_list_any].first->Signal(); } else if (waiting_for_events.count(awaiting_list_any_window) > 0) { - waiting_for_events[awaiting_list_any_window].second = cur.get(); + waiting_for_events[awaiting_list_any_window].second = cur.release(); waiting_for_events[awaiting_list_any_window].first->Signal(); } waiting_for_events_mu->unlock(); From 9a74c4ccad886ca5488fedff5b67a2fc509f2a76 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 18 Jul 2022 18:04:30 +0300 Subject: [PATCH 2/3] viewer: Use std::unique_ptr in waiting_for_events data structure The current usage of waiting_for_events is taking ownership of SVEvent pointer from a unique_ptr. This is error prone as all code paths using waiting_for_events need to ensure deletion. We fix it by using unique_ptr in waiting_for_events and all dependent code paths. --- src/classify/intproto.cpp | 4 +--- src/classify/shapeclassifier.cpp | 4 +--- src/textord/ccnontextdetect.cpp | 2 +- src/textord/colfind.cpp | 5 ++--- src/textord/strokewidth.cpp | 2 +- src/training/common/mastertrainer.cpp | 4 +--- src/training/unicharset/lstmtrainer.cpp | 2 +- src/viewer/scrollview.cpp | 27 ++++++++++--------------- src/viewer/scrollview.h | 3 ++- 9 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/classify/intproto.cpp b/src/classify/intproto.cpp index 373a967257..0638e59907 100644 --- a/src/classify/intproto.cpp +++ b/src/classify/intproto.cpp @@ -1163,12 +1163,11 @@ void FillPPLinearBits(uint32_t ParamTable[NUM_PP_BUCKETS][WERDS_PER_PP_VECTOR], CLASS_ID Classify::GetClassToDebug(const char *Prompt, bool *adaptive_on, bool *pretrained_on, int *shape_id) { tprintf("%s\n", Prompt); - SVEvent *ev; SVEventType ev_type; int unichar_id = INVALID_UNICHAR_ID; // Wait until a click or popup event. do { - ev = IntMatchWindow->AwaitEvent(SVET_ANY); + auto ev = IntMatchWindow->AwaitEvent(SVET_ANY); ev_type = ev->type; if (ev_type == SVET_POPUP) { if (ev->command_id == IDA_SHAPE_INDEX) { @@ -1214,7 +1213,6 @@ CLASS_ID Classify::GetClassToDebug(const char *Prompt, bool *adaptive_on, bool * } } } - delete ev; } while (ev_type != SVET_CLICK); return 0; } /* GetClassToDebug */ diff --git a/src/classify/shapeclassifier.cpp b/src/classify/shapeclassifier.cpp index da0ddb3bc1..11a6168e84 100644 --- a/src/classify/shapeclassifier.cpp +++ b/src/classify/shapeclassifier.cpp @@ -115,7 +115,6 @@ void ShapeClassifier::DebugDisplay(const TrainingSample &sample, Image page_pix, std::vector results; // Debug classification until the user quits. const UNICHARSET &unicharset = GetUnicharset(); - SVEvent *ev; SVEventType ev_type; do { std::vector windows; @@ -135,7 +134,7 @@ void ShapeClassifier::DebugDisplay(const TrainingSample &sample, Image page_pix, UNICHAR_ID old_unichar_id; do { old_unichar_id = unichar_id; - ev = debug_win->AwaitEvent(SVET_ANY); + auto ev = debug_win->AwaitEvent(SVET_ANY); ev_type = ev->type; if (ev_type == SVET_POPUP) { if (unicharset.contains_unichar(ev->parameter)) { @@ -144,7 +143,6 @@ void ShapeClassifier::DebugDisplay(const TrainingSample &sample, Image page_pix, tprintf("Char class '%s' not found in unicharset", ev->parameter); } } - delete ev; } while (unichar_id == old_unichar_id && ev_type != SVET_CLICK && ev_type != SVET_DESTROY); for (auto window : windows) { delete window; diff --git a/src/textord/ccnontextdetect.cpp b/src/textord/ccnontextdetect.cpp index 9fb2fd7294..26ae1656fb 100644 --- a/src/textord/ccnontextdetect.cpp +++ b/src/textord/ccnontextdetect.cpp @@ -134,7 +134,7 @@ Image CCNonTextDetect::ComputeNonTextMask(bool debug, Image photo_map, TO_BLOCK #endif // !GRAPHICS_DISABLED pixWrite("junkccphotomask.png", pix, IFF_PNG); #ifndef GRAPHICS_DISABLED - delete win->AwaitEvent(SVET_DESTROY); + win->AwaitEvent(SVET_DESTROY); delete win; #endif // !GRAPHICS_DISABLED } diff --git a/src/textord/colfind.cpp b/src/textord/colfind.cpp index 9e8558bfd3..3e97c2858e 100644 --- a/src/textord/colfind.cpp +++ b/src/textord/colfind.cpp @@ -440,7 +440,7 @@ int ColumnFinder::FindBlocks(PageSegMode pageseg_mode, Image scaled_color, int s DisplayTabVectors(window); } if (window != nullptr && textord_tabfind_show_partitions > 1) { - delete window->AwaitEvent(SVET_DESTROY); + window->AwaitEvent(SVET_DESTROY); } } } @@ -476,7 +476,7 @@ int ColumnFinder::FindBlocks(PageSegMode pageseg_mode, Image scaled_color, int s bool waiting = false; do { waiting = false; - SVEvent *event = blocks_win_->AwaitEvent(SVET_ANY); + auto event = blocks_win_->AwaitEvent(SVET_ANY); if (event->type == SVET_INPUT && event->parameter != nullptr) { if (*event->parameter == 'd') { result = -1; @@ -488,7 +488,6 @@ int ColumnFinder::FindBlocks(PageSegMode pageseg_mode, Image scaled_color, int s } else { waiting = true; } - delete event; } while (waiting); } #endif // !GRAPHICS_DISABLED diff --git a/src/textord/strokewidth.cpp b/src/textord/strokewidth.cpp index 8cf23db19c..d8f52f609c 100644 --- a/src/textord/strokewidth.cpp +++ b/src/textord/strokewidth.cpp @@ -123,7 +123,7 @@ StrokeWidth::StrokeWidth(int gridsize, const ICOORD &bleft, const ICOORD &tright StrokeWidth::~StrokeWidth() { #ifndef GRAPHICS_DISABLED if (widths_win_ != nullptr) { - delete widths_win_->AwaitEvent(SVET_DESTROY); + widths_win_->AwaitEvent(SVET_DESTROY); if (textord_tabfind_only_strokewidths) { exit(0); } diff --git a/src/training/common/mastertrainer.cpp b/src/training/common/mastertrainer.cpp index c45960bded..05b54b5dc7 100644 --- a/src/training/common/mastertrainer.cpp +++ b/src/training/common/mastertrainer.cpp @@ -785,9 +785,8 @@ void MasterTrainer::DisplaySamples(const char *unichar_str1, int cloud_font, ScrollView *s_window = CreateFeatureSpaceWindow("Samples", 100, 500); SVEventType ev_type; do { - SVEvent *ev; // Wait until a click or popup event. - ev = f_window->AwaitEvent(SVET_ANY); + auto ev = f_window->AwaitEvent(SVET_ANY); ev_type = ev->type; if (ev_type == SVET_CLICK) { int feature_index = feature_space.XYToFeatureIndex(ev->x, ev->y); @@ -801,7 +800,6 @@ void MasterTrainer::DisplaySamples(const char *unichar_str1, int cloud_font, s_window->Update(); } } - delete ev; } while (ev_type != SVET_DESTROY); } #endif // !GRAPHICS_DISABLED diff --git a/src/training/unicharset/lstmtrainer.cpp b/src/training/unicharset/lstmtrainer.cpp index 500cea76e8..0ebad4de1b 100644 --- a/src/training/unicharset/lstmtrainer.cpp +++ b/src/training/unicharset/lstmtrainer.cpp @@ -891,7 +891,7 @@ Trainability LSTMTrainer::TrainOnLine(const ImageData *trainingdata, } #ifndef GRAPHICS_DISABLED if (debug_interval_ == 1 && debug_win_ != nullptr) { - delete debug_win_->AwaitEvent(SVET_CLICK); + debug_win_->AwaitEvent(SVET_CLICK); } #endif // !GRAPHICS_DISABLED // Roll the memory of past means. diff --git a/src/viewer/scrollview.cpp b/src/viewer/scrollview.cpp index 3e3517b638..087dabe8ec 100644 --- a/src/viewer/scrollview.cpp +++ b/src/viewer/scrollview.cpp @@ -56,8 +56,8 @@ struct SVPolyLineBuffer { static std::map svmap; static std::mutex *svmap_mu; // A map of all semaphores waiting for a specific event on a specific window. -static std::map, std::pair> - waiting_for_events; +static std::map, + std::pair>> waiting_for_events; static std::mutex *waiting_for_events_mu; SVEvent *SVEvent::copy() const { @@ -158,13 +158,13 @@ void ScrollView::MessageReceiver() { SVET_ANY); waiting_for_events_mu->lock(); if (waiting_for_events.count(awaiting_list) > 0) { - waiting_for_events[awaiting_list].second = cur.release(); + waiting_for_events[awaiting_list].second = std::move(cur); waiting_for_events[awaiting_list].first->Signal(); } else if (waiting_for_events.count(awaiting_list_any) > 0) { - waiting_for_events[awaiting_list_any].second = cur.release(); + waiting_for_events[awaiting_list_any].second = std::move(cur); waiting_for_events[awaiting_list_any].first->Signal(); } else if (waiting_for_events.count(awaiting_list_any_window) > 0) { - waiting_for_events[awaiting_list_any_window].second = cur.release(); + waiting_for_events[awaiting_list_any_window].second = std::move(cur); waiting_for_events[awaiting_list_any_window].first->Signal(); } waiting_for_events_mu->unlock(); @@ -367,8 +367,7 @@ ScrollView::~ScrollView() { // So the event handling thread can quit. SendMsg("destroy()"); - SVEvent *sve = AwaitEvent(SVET_DESTROY); - delete sve; + AwaitEvent(SVET_DESTROY); svmap_mu->lock(); svmap[window_id_] = nullptr; svmap_mu->unlock(); @@ -442,19 +441,19 @@ void ScrollView::SetEvent(const SVEvent *svevent) { /// Block until an event of the given type is received. /// Note: The calling function is responsible for deleting the returned /// SVEvent afterwards! -SVEvent *ScrollView::AwaitEvent(SVEventType type) { +std::unique_ptr ScrollView::AwaitEvent(SVEventType type) { // Initialize the waiting semaphore. auto *sem = new SVSemaphore(); std::pair ea(this, type); waiting_for_events_mu->lock(); - waiting_for_events[ea] = std::pair(sem, (SVEvent *)nullptr); + waiting_for_events[ea] = {sem, nullptr}; waiting_for_events_mu->unlock(); // Wait on it, but first flush. stream_->Flush(); sem->Wait(); // Process the event we got woken up for (its in waiting_for_events pair). waiting_for_events_mu->lock(); - SVEvent *ret = waiting_for_events[ea].second; + auto ret = std::move(waiting_for_events[ea].second); waiting_for_events.erase(ea); delete sem; waiting_for_events_mu->unlock(); @@ -734,23 +733,19 @@ void ScrollView::Brush(Color color) { // Shows a modal Input Dialog which can return any kind of String char *ScrollView::ShowInputDialog(const char *msg) { SendMsg("showInputDialog(\"%s\")", msg); - SVEvent *ev; // wait till an input event (all others are thrown away) - ev = AwaitEvent(SVET_INPUT); + auto ev = AwaitEvent(SVET_INPUT); char *p = new char[strlen(ev->parameter) + 1]; strcpy(p, ev->parameter); - delete ev; return p; } // Shows a modal Yes/No Dialog which will return 'y' or 'n' int ScrollView::ShowYesNoDialog(const char *msg) { SendMsg("showYesNoDialog(\"%s\")", msg); - SVEvent *ev; // Wait till an input event (all others are thrown away) - ev = AwaitEvent(SVET_INPUT); + auto ev = AwaitEvent(SVET_INPUT); int a = ev->parameter[0]; - delete ev; return a; } diff --git a/src/viewer/scrollview.h b/src/viewer/scrollview.h index a11ebed91c..f583eba579 100644 --- a/src/viewer/scrollview.h +++ b/src/viewer/scrollview.h @@ -36,6 +36,7 @@ #include #include +#include #include namespace tesseract { @@ -186,7 +187,7 @@ class TESS_API ScrollView { void AddEventHandler(SVEventHandler *listener); // Block until an event of the given type is received. - SVEvent *AwaitEvent(SVEventType type); + std::unique_ptr AwaitEvent(SVEventType type); /******************************************************************************* * Getters and Setters From 0107687a9b85e288456ef3ed026e1c2eb86db029 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 18 Jul 2022 18:04:31 +0300 Subject: [PATCH 3/3] viewer: Use std::unique_ptr in event_table_ data structure --- src/viewer/scrollview.cpp | 32 ++++++++++---------------------- src/viewer/scrollview.h | 4 ++-- 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/src/viewer/scrollview.cpp b/src/viewer/scrollview.cpp index 087dabe8ec..f5336c589a 100644 --- a/src/viewer/scrollview.cpp +++ b/src/viewer/scrollview.cpp @@ -60,8 +60,8 @@ static std::map, std::pair>> waiting_for_events; static std::mutex *waiting_for_events_mu; -SVEvent *SVEvent::copy() const { - auto *any = new SVEvent; +std::unique_ptr SVEvent::copy() const { + auto any = std::unique_ptr(new SVEvent); any->command_id = command_id; any->counter = counter; any->parameter = new char[strlen(parameter) + 1]; @@ -319,12 +319,9 @@ void ScrollView::Initialize(const char *name, int x_pos, int y_pos, int x_size, /// Sits and waits for events on this window. void ScrollView::StartEventHandler() { - SVEvent *new_event; - for (;;) { stream_->Flush(); semaphore_->Wait(); - new_event = nullptr; int serial = -1; int k = -1; mutex_.lock(); @@ -332,25 +329,22 @@ void ScrollView::StartEventHandler() { for (int i = 0; i < SVET_COUNT; i++) { if (event_table_[i] != nullptr && (serial < 0 || event_table_[i]->counter < serial)) { - new_event = event_table_[i]; serial = event_table_[i]->counter; k = i; } } // If we didn't find anything we had an old alarm and just sleep again. - if (new_event != nullptr) { - event_table_[k] = nullptr; + if (k != -1) { + auto new_event = std::move(event_table_[k]); mutex_.unlock(); if (event_handler_ != nullptr) { - event_handler_->Notify(new_event); + event_handler_->Notify(new_event.get()); } 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. return; } - delete new_event; // Delete the pointer after it has been processed. } else { mutex_.unlock(); } @@ -382,9 +376,6 @@ ScrollView::~ScrollView() { } delete semaphore_; delete points_; - for (auto &i : event_table_) { - delete i; - } #endif // !GRAPHICS_DISABLED } @@ -424,18 +415,15 @@ void ScrollView::Signal() { void ScrollView::SetEvent(const SVEvent *svevent) { // Copy event - SVEvent *any = svevent->copy(); - SVEvent *specific = svevent->copy(); + auto any = svevent->copy(); + auto specific = svevent->copy(); any->counter = specific->counter + 1; // Place both events into the queue. std::lock_guard guard(mutex_); - // Delete the old objects.. - delete event_table_[specific->type]; - delete event_table_[SVET_ANY]; - // ...and put the new ones in the table. - event_table_[specific->type] = specific; - event_table_[SVET_ANY] = any; + + event_table_[specific->type] = std::move(specific); + event_table_[SVET_ANY] = std::move(any); } /// Block until an event of the given type is received. diff --git a/src/viewer/scrollview.h b/src/viewer/scrollview.h index f583eba579..b9d06ef0af 100644 --- a/src/viewer/scrollview.h +++ b/src/viewer/scrollview.h @@ -70,7 +70,7 @@ struct SVEvent { ~SVEvent() { delete[] parameter; } - SVEvent *copy() const; + std::unique_ptr copy() const; SVEventType type = SVET_DESTROY; // What kind of event. ScrollView *window = nullptr; // Window event relates to. char *parameter = nullptr; // Any string that might have been passed as argument. @@ -414,7 +414,7 @@ class TESS_API ScrollView { static SVNetwork *stream_; // Table of all the currently queued events. - SVEvent *event_table_[SVET_COUNT]; + std::unique_ptr event_table_[SVET_COUNT]; // Mutex to access the event_table_ in a synchronized fashion. std::mutex mutex_;