After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 793026 - possible mem leak in g_mutex_impl_new
possible mem leak in g_mutex_impl_new
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gthread
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2018-01-30 11:02 UTC by Daniele
Modified: 2018-02-06 15:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gthread: Fix a typo in an #ifdef on the non-native mutex path (1.48 KB, patch)
2018-01-31 22:51 UTC, Philip Withnall
committed Details | Review

Description Daniele 2018-01-30 11:02:08 UTC
In g_mutex_impl_new we have

#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
  {
    pthread_mutexattr_t attr;
    pthread_mutexattr_init (&attr);
    pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_ADAPTIVE_NP);
    pattr = &attr;
  }
#endif

attr context is within PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP and braces.

Later on, to destroy attr we have

#ifdef PTHREAD_ADAPTIVE_MUTEX_NP
  pthread_mutexattr_destroy (&attr);
#endif

which is within PTHREAD_ADAPTIVE_MUTEX_NP so not PTHREAD_ADAPTIVE_MUTEX_INITIALIZED_NP but even if we use the correct ifdef, having the declaration of attr inside braces, it is not in the same scope
Comment 1 Philip Withnall 2018-01-31 22:51:28 UTC
Created attachment 367738 [details] [review]
gthread: Fix a typo in an #ifdef on the non-native mutex path

This seems to have been present since the code was introduced in commit
cedc82290f860683d695d0c5326db153893eec21. The attr variable is defined
under one #ifdef, but destroyed under another, which doesn’t make any
sense. The second #ifdef variable is actually an enum value, rather than
the static initialiser value which makes more sense in the context.

Note that GMutex used to be statically initialised to the value of
PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP in gthread.h, before this was
reworked in commit e081eadda598bc708fbf9dd53a190fc3b0e7fa76.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
Comment 2 Philip Withnall 2018-01-31 22:52:31 UTC
This does indeed look like a bug, thanks, although it’s not one which will typically be hit, because it’s in the non-native mutex implementation path. Typically, GLib on Linux will use native futexes.
Comment 3 Daniele 2018-02-01 07:49:13 UTC
Just for clarification: attr declaration is surrounded by braces. Doesn't it mean that it's scope is visible only there and not inside the second ifdef where we destroy it?

I thought we should remove the braces in the declaration or do something like this

#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
  {
    pthread_mutexattr_t attr;
    pthread_mutexattr_init (&attr);
    pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_ADAPTIVE_NP);
    pattr = &attr;
#endif

  ...

#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
    pthread_mutexattr_destroy (&attr);
  }
#endif
Comment 4 Philip Withnall 2018-02-01 10:45:05 UTC
(In reply to Daniele from comment #3)
> Just for clarification: attr declaration is surrounded by braces. Doesn't it
> mean that it's scope is visible only there and not inside the second ifdef
> where we destroy it?

What version of GLib are you looking at? The braces were removed in commit 03a82ce898ea435efd73a250d989f8dc6249aaf8, in 2014.

git master currently looks like this: https://git.gnome.org/browse/glib/tree/glib/gthread-posix.c
Comment 5 Daniele 2018-02-01 10:51:47 UTC
Sorry. Just realized my link was on github and not on git.gnome.

https://github.com/djdeath/glib/blob/android/glib/gthread-posix.c

Suppose this is the android branch.
Comment 6 Philip Withnall 2018-02-01 11:02:54 UTC
I have no idea what that branch is meant to do (it’s not official). It’s pretty old. I believe Android support works on master when building with Meson, although I haven’t tested it. If you are trying to use GLib on Android, and have problems, please file more bugs or get in touch with people on IRC.

In any case, there is still a bug here on git master, which my patch should fix. It’s waiting for another developer to review it.
Comment 7 Matthias Clasen 2018-02-06 15:50:22 UTC
Review of attachment 367738 [details] [review]:

sure
Comment 8 Philip Withnall 2018-02-06 15:52:55 UTC
Pushed to master, thanks Matthias!

Attachment 367738 [details] pushed as 79d9ea2 - gthread: Fix a typo in an #ifdef on the non-native mutex path