Bug 239520 - Possible memory leak in newlocale/uselocale
Summary: Possible memory leak in newlocale/uselocale
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: 12.2-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-12-14 19:15 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.
Comment 12 Alexander Shikov 2020-12-11 08:06:11 UTC
Hi!

It seems that issue was fixed partially. C code above does not reproduce it on 12.2-RELEASE, but perl scripts still leak memory.

For example, running this code:
#!/usr/local/bin/perl -w

use strict;
use Sys::Syslog qw(:standard :macros);

openlog('leak-demo', LOG_NDELAY, LOG_LOCAL2);

my $c = 0;

my @chars = ("A".."Z", "a".."z", "0".."9", "." , "-", "_" );

while ($c++ <= 10000) {
        my $string = '';
        $string .= $chars[rand @chars] for 1..32;
        for (1..2048) {
                syslog(LOG_DEBUG, "DEBUG LOG %d/%d RANDOM STRING: %s", $c, $_, $string );
        }
        sleep 1;
}

closelog();

... with locale settings like these:
LANG=uk_UA.UTF-8
LC_CTYPE="uk_UA.UTF-8"
LC_COLLATE="uk_UA.UTF-8"
LC_TIME="uk_UA.UTF-8"
LC_NUMERIC="uk_UA.UTF-8"
LC_MONETARY="uk_UA.UTF-8"
LC_MESSAGES="uk_UA.UTF-8"
LC_ALL=

... results to memory leak:

last pid: 39263;  load averages:  0.46,  0.36,  0.28                                                                                                         up 0+15:53:09  09:55:04
49 processes:  1 running, 48 sleeping
CPU: 24.1% user,  0.0% nice, 11.4% system,  0.6% interrupt, 63.9% idle
Mem: 770M Active, 558M Inact, 2068K Laundry, 447M Wired, 200M Buf, 231M Free
Swap: 4096M Total, 52M Used, 4044M Free, 1% Inuse

  PID USERNAME    THR PRI NICE   SIZE    RES STATE    C   TIME    WCPU COMMAND
39237 root          1  35    0   659M   646M nanslp   1   0:18  18.06% perl
[...]

Running the same script with "env LANG=C" does not leak memory.
Comment 13 Mark Johnston freebsd_committer 2020-12-11 15:34:33 UTC
(In reply to Alexander Shikov from comment #12)
This does not trigger any leaks on head.  The problem is actually the one fixed in PR 249416, which did not make it to 12.2.  I did not request an EN because there was only a single report of the problem and it was worked around in a port, but perhaps we should release one.
Comment 14 Alexander Shikov 2020-12-14 11:06:15 UTC
(In reply to Mark Johnston from comment #13)
Dear Mark, thank you for feedback. We definitely would like to have this fixed in 12-STABLE. Thank you!
Comment 15 Mark Johnston freebsd_committer 2020-12-14 14:51:02 UTC
(In reply to Alexander Shikov from comment #14)
Well it is already fixed in stable/12, the question is whether it is worth releasing a patch for 12.2-RELEASE.
Comment 16 Alexander Shikov 2020-12-14 19:15:14 UTC
(In reply to Mark Johnston from comment #15)
It's up to you. We don't experience a deep impact on our services, it's just annoying when background scripts unexpectedly crash out of working hours.