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.
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.
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.
(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.
https://reviews.freebsd.org/D26522
(In reply to Craig Leres from comment #3) Thanks. Are you able to test it with the original application?
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
A commit references this bug: Author: markj Date: Fri Oct 23 14:47:33 UTC 2020 New revision: 366971 URL: https://svnweb.freebsd.org/changeset/base/366971 Log: MFC r366375: newlocale(3): Fix a memory leak. PR: 249416 Changes: _U stable/12/ stable/12/lib/libc/locale/newlocale.3 stable/12/lib/libc/locale/xlocale.c
So I suspect we'll want to issue an EN for this once 12.2 is released. I'll keep this PR open for now until after the release goes out.
Is it safe to assume this fix will be in 12.3? If so then we could wrap the workaround for devel/json-c with: .if ${OPSYS} == FreeBSD && ${OSVERSION} < 1203000 ... .endif As described in the json-c PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=249412
(In reply to Craig Leres from comment #9) Yes it will definitely be in 12.3. The change is already in the stable/12 branch, where the __FreeBSD_version value is currently 1202503, so you could perhaps apply the workaround only if ${OSVERSION} <= 1202503.
I see that a workaround was applied to devel/json-c so I'm just going to go ahead and close this PR. If there's some demand for an EN, we can issue one.