Bug 239520 - Possible memory leak in newlocale/uselocale
Summary: Possible memory leak in newlocale/uselocale
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: 12.0-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-30 07:00 UTC by Alexander Shikov
Modified: 2020-04-03 00:55 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Shikov 2019-07-30 07:00:55 UTC
Hello!

Running the binary compiled from the simple code

#include <locale.h>                                                                                                                                                   
                                                                                                                                                                      
int main() {                                                                                                                                                          
  while (1) {                                                                                                                                                         
    locale_t n = newlocale(LC_TIME, "en_AU.UTF-8", (locale_t)0);                                                                                                 
    locale_t old = uselocale(n);                                                                                                                                      
    uselocale(old);                                                                                                                                                   
    freelocale(n);                                                                                                                                                    
  }                                                                                                                                                                   
}                                                                                                                                                                     

results to rapid memory leak. It was tested on 12.0-RELEASE/amd64, 11.2-RELEASE/amd64 and 9.3-RELEASE-p53/i386.

First time the problem with memory leak was found with Perl 5.28 and Perl 5.30 on 12.0-RELEASE/amd64. The firt suspicion was about Perl, but further discussion came to this test C code. Discussion on rt.perl.org: https://rt.perl.org/Public/Bug/Display.html?id=134305
Comment 1 Baggio Kwok 2019-07-31 07:39:37 UTC
Looks like the following atomic operation is not working as expected when manipulating the reference count.

/usr/src/lib/libc/locale/xlocale_private.h:183
177 __attribute__((unused)) static void
178 xlocale_release(void *val)
179 {
180   struct xlocale_refcounted *obj = val;
181   long count;
182 
183   count = atomic_fetchadd_long(&(obj->retain_count), -1) - 1;
184   if (count < 0 && obj->destructor != NULL)
185     obj->destructor(obj);
186 }
Comment 2 Baggio Kwok 2019-07-31 07:53:45 UTC
1. atomic_fetchadd_long returns previous value, so I think /usr/src/lib/libc/locale/xlocale_private.h:184 should check for count == 0?

2. looks like atomic_fetchadd_long is expecting u_long which is unsigned, would passing -1 in /usr/src/lib/libc/locale/xlocale_private.h:183 works as we expected?

__________________________________________________
sys/amd64/include/atomic.h

241 /*
242  * Atomically add the value of v to the long integer pointed to by p and return
243  * the previous value of *p.
244  */
245 static __inline u_long
246 atomic_fetchadd_long(volatile u_long *p, u_long v)
247 {
248 
249   __asm __volatile(
250   " " MPLOCKED "    "
251   " xaddq %0,%1 ;   "
252   "# atomic_fetchadd_long"
253   : "+r" (v),     /* 0 */
254     "+m" (*p)     /* 1 */
255   : : "cc");
256   return (v);
257 }
Comment 3 Baggio Kwok 2019-07-31 08:00:33 UTC
Ok, a quick check shows atomic_fetchadd_long(_, -1) [2] should works properly, both linuxkpi and nginx do it like that.
Comment 4 James E Keenan 2020-03-20 16:41:46 UTC
(In reply to Baggio Kwok from comment #3)

Are there plans to address this issue?  It is still causing memory leaks downstream.

Thank you very much.
Jim Keenan
Comment 5 Mark Johnston freebsd_committer 2020-03-20 16:57:42 UTC
I think the refcount bug is real, but there is another leak of locale file mappings, apparently not caught by r356569.
Comment 6 Mark Johnston freebsd_committer 2020-03-20 17:20:07 UTC
In some cases we call xlocale_release() on a structure with a refcount of 0, so really the test should be count <= 0.
Comment 7 Mark Johnston freebsd_committer 2020-03-20 19:55:18 UTC
(In reply to Mark Johnston from comment #6)
This is a red herring, libc is doing locale reference counting properly.  The real problem is that set_thread_locale() is using pthread_getspecific() to get the old locale even in a single-threaded program, and that doesn't work.
Comment 8 commit-hook freebsd_committer 2020-03-20 20:03:03 UTC
A commit references this bug:

Author: markj
Date: Fri Mar 20 20:02:53 UTC 2020
New revision: 359183
URL: https://svnweb.freebsd.org/changeset/base/359183

Log:
  Fix uselocale(3) to not leak a reference to the old locale.

  In a single-threaded program pthread_getspecific() always returns NULL,
  so the old locale would not end up being freed.

  PR:		239520
  MFC after:	2 weeks
  Sponsored by:	The FreeBSD Foundation

Changes:
  head/lib/libc/locale/xlocale.c
Comment 9 commit-hook freebsd_committer 2020-04-03 00:33:17 UTC
A commit references this bug:

Author: markj
Date: Fri Apr  3 00:32:48 UTC 2020
New revision: 359583
URL: https://svnweb.freebsd.org/changeset/base/359583

Log:
  MFC r359183:
  Fix uselocale(3) to not leak a reference to the old locale.

  PR:	239520

Changes:
_U  stable/12/
  stable/12/lib/libc/locale/xlocale.c
Comment 10 commit-hook freebsd_committer 2020-04-03 00:46:21 UTC
A commit references this bug:

Author: markj
Date: Fri Apr  3 00:38:12 UTC 2020
New revision: 359584
URL: https://svnweb.freebsd.org/changeset/base/359584

Log:
  MFC r359183:
  Fix uselocale(3) to not leak a reference to the old locale.

  PR:	239520

Changes:
_U  stable/11/
  stable/11/lib/libc/locale/xlocale.c
Comment 11 Mark Johnston freebsd_committer 2020-04-03 00:55:01 UTC
This will be fixed in 11.4 and 12.2.  Thanks for the report.