Re: [PATCH v3 09/18] perf ui: Update use of pthread mutex

[Adrian Hunter <adrian.hunter@intel.com>]

On 26/08/22 20:45, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 10:22 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 26/08/22 19:02, Ian Rogers wrote:
>>> On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 24/08/22 18:38, Ian Rogers wrote:
>>>>> Switch to the use of mutex wrappers that provide better error checking.
>>>>>
>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>> ---
>>>>>  tools/perf/ui/browser.c           | 20 ++++++++++----------
>>>>>  tools/perf/ui/browsers/annotate.c |  2 +-
>>>>>  tools/perf/ui/setup.c             |  5 +++--
>>>>>  tools/perf/ui/tui/helpline.c      |  5 ++---
>>>>>  tools/perf/ui/tui/progress.c      |  8 ++++----
>>>>>  tools/perf/ui/tui/setup.c         |  8 ++++----
>>>>>  tools/perf/ui/tui/util.c          | 18 +++++++++---------
>>>>>  tools/perf/ui/ui.h                |  4 ++--
>>>>>  8 files changed, 35 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>>>>> index fa5bd5c20e96..78fb01d6ad63 100644
>>>>> --- a/tools/perf/ui/browser.c
>>>>> +++ b/tools/perf/ui/browser.c
>>>>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>>>
>>>>>  void ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>>>  {
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       __ui_browser__show_title(browser, title);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>  }
>>>>>
>>>>>  int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>>
>>>>>       browser->refresh_dimensions(browser);
>>>>>
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       __ui_browser__show_title(browser, title);
>>>>>
>>>>>       browser->title = title;
>>>>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>>       va_end(ap);
>>>>>       if (err > 0)
>>>>>               ui_helpline__push(browser->helpline);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>       return err ? 0 : -1;
>>>>>  }
>>>>>
>>>>>  void ui_browser__hide(struct ui_browser *browser)
>>>>>  {
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       ui_helpline__pop();
>>>>>       zfree(&browser->helpline);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>  }
>>>>>
>>>>>  static void ui_browser__scrollbar_set(struct ui_browser *browser)
>>>>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>>>>>
>>>>>  int ui_browser__refresh(struct ui_browser *browser)
>>>>>  {
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       __ui_browser__refresh(browser);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>
>>>>>       return 0;
>>>>>  }
>>>>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>>>>>       while (1) {
>>>>>               off_t offset;
>>>>>
>>>>> -             pthread_mutex_lock(&ui__lock);
>>>>> +             mutex_lock(&ui__lock);
>>>>>               err = __ui_browser__refresh(browser);
>>>>>               SLsmg_refresh();
>>>>> -             pthread_mutex_unlock(&ui__lock);
>>>>> +             mutex_unlock(&ui__lock);
>>>>>               if (err < 0)
>>>>>                       break;
>>>>>
>>>>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>>>>> index 44ba900828f6..b8747e8dd9ea 100644
>>>>> --- a/tools/perf/ui/browsers/annotate.c
>>>>> +++ b/tools/perf/ui/browsers/annotate.c
>>>>> @@ -8,11 +8,11 @@
>>>>>  #include "../../util/hist.h"
>>>>>  #include "../../util/sort.h"
>>>>>  #include "../../util/map.h"
>>>>> +#include "../../util/mutex.h"
>>>>>  #include "../../util/symbol.h"
>>>>>  #include "../../util/evsel.h"
>>>>>  #include "../../util/evlist.h"
>>>>>  #include <inttypes.h>
>>>>> -#include <pthread.h>
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/string.h>
>>>>>  #include <linux/zalloc.h>
>>>>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>>>>> index 700335cde618..25ded88801a3 100644
>>>>> --- a/tools/perf/ui/setup.c
>>>>> +++ b/tools/perf/ui/setup.c
>>>>> @@ -1,5 +1,4 @@
>>>>>  // SPDX-License-Identifier: GPL-2.0
>>>>> -#include <pthread.h>
>>>>>  #include <dlfcn.h>
>>>>>  #include <unistd.h>
>>>>>
>>>>> @@ -8,7 +7,7 @@
>>>>>  #include "../util/hist.h"
>>>>>  #include "ui.h"
>>>>>
>>>>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
>>>>> +struct mutex ui__lock;
>>>>>  void *perf_gtk_handle;
>>>>>  int use_browser = -1;
>>>>>
>>>>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>>>>>
>>>>>  void setup_browser(bool fallback_to_pager)
>>>>>  {
>>>>> +     mutex_init(&ui__lock);
>>>>>       if (use_browser < 2 && (!isatty(1) || dump_trace))
>>>>>               use_browser = 0;
>>>>>
>>>>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>>>>>       default:
>>>>>               break;
>>>>>       }
>>>>> +     mutex_destroy(&ui__lock);
>>>>
>>>> Looks like exit_browser() can be called even when setup_browser()
>>>> was never called.
>>>>
>>>> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
>>>> pthread won't notice.
>>>
>>> Memory sanitizer will notice some cases of this and so I didn't want
>>> to code defensively around exit being called ahead of setup.
>>
>> I am not sure you understood.
>>
>> As I wrote, exit_browser() can be called even when setup_browser()
>> was never called, so it is not defensive programming, it is necessary
>> programming that you only get away without doing because
>> PTHREAD_MUTEX_INITIALIZER is all zeros.
> 
> Why are we here:
> 1) there is a memory leak
> 2) I fix the memory and trigger a use after free
> 3) I invent a reference count checker, use it to fix the memory leak,
> use after free and missing locks - the patch for this in 10s of lines
> long
> 4) when adding the lock fixes I defensively add error checking to the
> mutex involved - mainly because I was scared I could introduce a
> deadlock
> 5) I get asked to generalize this
> 6) GSoC contributor picks up and puts this down
> 7) I pull together the contributor's work
> 8) I get asked to turn a search and replace 4 patch fix into an unwieldy patch
> 9) I worry about the sanity of the change and add lock checking from clang
> 10) I end up trying to fix perf-sched who for some reason thought it
> was perfectly valid to have threads blocked on mutexes that were
> deallocated on the stack.
> 11) the UI code was written with a view that exiting something not
> setup somehow made sense
> 
> I am drawing a line at fixing perf sched and the UI code. We can drop
> this patch and keep things as a pthread_mutex_t, similarly for
> perf-sched. I have gone about as far as I'm prepared to for the sake
> of a 10s of line memory leak fix. Some private thoughts are, it would
> be useful if review comments could be constructive, hey do this not
> that, and not simply commenting on change or trying to shoehorn vast
> amounts of tech debt clean up onto simple fixes.

If you want help, you only need ask.

Below seems adequate for now, at least logically, but maybe it
would confuse clang thread-safety analysis?

diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
index 25ded88801a3..6d81be6a349e 100644
--- a/tools/perf/ui/setup.c
+++ b/tools/perf/ui/setup.c
@@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
 	return 0;
 }
 
+/*
+ * exit_browser() can get called without setup_browser() having been called
+ * first, so it is necessary to keep track of whether ui__lock has been
+ * initialized.
+ */
+static bool ui__lock_initialized;
+
 void setup_browser(bool fallback_to_pager)
 {
 	mutex_init(&ui__lock);
+	ui__lock_initialized = true;
 	if (use_browser < 2 && (!isatty(1) || dump_trace))
 		use_browser = 0;
 
@@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
 	default:
 		break;
 	}
-	mutex_destroy(&ui__lock);
+	if (ui__lock_initialized)
+		mutex_destroy(&ui__lock);
 }