Bug 249416 - Each call to newlocale(3) with a non-null base argument results in additional memory usage
Summary: Each call to newlocale(3) with a non-null base argument results in additional...
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.1-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-18 01:33 UTC by Craig Leres
Modified: 2020-10-02 18:37 UTC (History)
1 user (show)

See Also:


Attachments
newlocale() test program (3.83 KB, text/plain)
2020-09-18 01:33 UTC, Craig Leres
no flags Details
proposed patch (801 bytes, patch)
2020-09-19 18:09 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Craig Leres freebsd_committer 2020-09-18 01:33:46 UTC
Created attachment 218038 [details]
newlocale() test program

There appears to be a memory usage issue with newlocale(3) when called with a base argument. This recently started showing symptoms in devel/json-c programs using version 0.14 and higher which started using newlocale() due to a change in the cmake configuration. Here is the devel/json-c PR:

    https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=249412

Here is the upstream ticket I created:

    https://github.com/json-c/json-c/issues/668

Eric Hawicz responded, suggesting that FreeBSD's implementation of newlocale() might be causing the issue. His helpful test program confirms this. Attached to this PR is a modified version that uses sysctl() to automatically track and report changes to rss and vsz.

Eric points out that FreeBSD and linux man pages describe different behavior; FreeBSD:

    https://www.freebsd.org/cgi/man.cgi?query=newlocale&sektion=3&manpath=freebsd-release-ports
    "Creates a new locale, inheriting some properties from an existing locale."

Note that there do not appear to be uses of newlocale() with a non-null base argument in the current source tree; linux:

    https://man7.org/linux/man-pages/man3/newlocale.3.html
    "The newlocale() function creates a new locale object, or modifies an existing object..."

And that posix says:

    https://pubs.opengroup.org/onlinepubs/9699919799/functions/newlocale.html
    "The newlocale() function shall create a new locale object or modify an existing one."

I did some testing with versions of lib/libc/locale going back to 2012 and all had this issue.
Comment 1 Mark Johnston freebsd_committer 2020-09-19 18:03:44 UTC
I suspect it'll be ok to make newlocale() release a reference on the base locale.  I tested a patch that does exactly this and it appears to fix the leak.
Comment 2 Mark Johnston freebsd_committer 2020-09-19 18:09:03 UTC
Created attachment 218084 [details]
proposed patch

Here is a proposed patch.  I am not certain about it yet, so please be careful if you want to test it.  It is missing a man page update.  An exp-run may be warranted, I'm not sure.  The patch may create bugs in anything that relies on being able to access the base locale after newlocale() returns.  Linux explicitly prohibits this, so I guess we should be ok most of the time.
Comment 3 Craig Leres freebsd_committer 2020-09-21 17:55:56 UTC
(In reply to Mark Johnston from comment #2)
The patch tests good for me on my 13.0-CURRENT box; both my localetest and jsontest programs went from memory growth to none just by installing an updated libc.so.
Comment 4 Mark Johnston freebsd_committer 2020-09-22 16:47:03 UTC
https://reviews.freebsd.org/D26522
Comment 5 Mark Johnston freebsd_committer 2020-09-22 16:47:32 UTC
(In reply to Craig Leres from comment #3)
Thanks.  Are you able to test it with the original application?
Comment 6 commit-hook freebsd_committer 2020-10-02 18:36:05 UTC
A commit references this bug:

Author: markj
Date: Fri Oct  2 18:35:56 UTC 2020
New revision: 366375
URL: https://svnweb.freebsd.org/changeset/base/366375

Log:
  newlocale(3): Fix a memory leak.

  newlocale() optionally takes a "base" locale, from which components not
  specified in the mask are inherited.  POSIX says that newlocale() may
  modify "base" and return it, or free "base" and return a newly allocated
  locale.  We were not doing either, so applications which use newlocale()
  to modify an existing base locale end up leaking memory on FreeBSD.

  This diff fixes the leak by releasing a reference to the base locale
  before returning.  This is less efficient than modifying "base"
  directly, but is simpler for an initial bug fix.  Also, update the man
  page to clarify behaviour with respect to "base".

  PR:		249416
  MFC after:	3 weeks
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D26522

Changes:
  head/lib/libc/locale/newlocale.3
  head/lib/libc/locale/xlocale.c