Bug 249412

Summary: devel/json-c: Avoid use of newlocale(3) that results in increasing memory usage
Product: Ports & Packages Reporter: Craig Leres <leres>
Component: Individual Port(s)Assignee: Craig Leres <leres>
Status: Closed FIXED    
Severity: Affects Only Me CC: sunpoet
Priority: --- Keywords: buildisok, patch
Version: LatestFlags: bugzilla: maintainer-feedback? (sunpoet)
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch
leres: maintainer-approval?
test program
none
test program Makefile none

Description Craig Leres freebsd_committer freebsd_triage 2020-09-17 23:39:01 UTC
Starting with github hash bf29aa0 and in between the 0.13.1 and 0.14 releases, json-c uses newlocale(3) which appears to cause increased memory usage on each call when base is supplied.

We have a persistent daemon that repeatedly parses some trivial json from a piece of network equipment. This software was developed while the FreeBSD devel/json-c port was at 0.13.1 (or older). The json-c port was updated to 0.14 in May 2020 and to 0.15 in mid August. After the 0.15 update the daemon was rebuilt/installed and after ~12 hours of operation would crash due to memory exhaustion. Eventually we determined that downgrading to 0.13.1 solved the memory growth.

I wrote a test program that calls json_tokener_parse() followed by json_object_put() in a loop; rss slowly increases for every loop.

I opened this upstream ticket:

    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.

Looking at the first version of json-c to has the issue:

    https://github.com/json-c/json-c/commit/bf29aa0

HAVE_USELOCALE is defined when locale.h is present. This results in a different codepath in json_tokener.c where newlocale() is used instead of setlocale().

I'll submit a seperate PR for the newlocale() issue.

The attached patch comments out HAVE_USELOCALE in post-configure to avoid the use of newlocale(). Also attached is a json-c test program which uses sysctl() to automatically track and report changes to rss and vsz and clearly shows the issue.
Comment 1 Craig Leres freebsd_committer freebsd_triage 2020-09-17 23:39:32 UTC
Created attachment 218034 [details]
patch
Comment 2 Craig Leres freebsd_committer freebsd_triage 2020-09-17 23:40:17 UTC
Created attachment 218035 [details]
test program
Comment 3 Craig Leres freebsd_committer freebsd_triage 2020-09-17 23:40:39 UTC
Created attachment 218036 [details]
test program Makefile
Comment 4 Automation User 2020-10-02 00:36:04 UTC
Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/197164210
Comment 5 commit-hook freebsd_committer freebsd_triage 2020-10-07 22:12:06 UTC
A commit references this bug:

Author: leres
Date: Wed Oct  7 22:11:30 UTC 2020
New revision: 551672
URL: https://svnweb.freebsd.org/changeset/ports/551672

Log:
  devel/json-c: Avoid use of newlocale(3) that results in increasing memory usage

  The json-c distribution began using newlocale(3) starting with 0.14.
  Unfortunately the FreeBSD implementation is not posix compliant and
  when called with a base does not modify and return it nor does it
  free it; it always allocates and returns a new locale, leaking the
  base locale. See the PR for a test program that demonstrates the
  json-c issue. Here is the upstream github issue:

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

  The fix to the port is to comment out HAVE_USELOCALE in post-configure
  and avoid the use newlocale() for now.

  A fix for newlocale(3) is in progress:

      https://reviews.freebsd.org/D26522

  So it is likely this problem will be solved in time for 12.3-RELEASE.

  PR:		249412
  Approved by:	sunpoet (maintainer timeout, 3 weeks)

Changes:
  head/devel/json-c/Makefile
Comment 6 Craig Leres freebsd_committer freebsd_triage 2020-10-07 22:12:56 UTC
I committed the patch.
Comment 7 Po-Chuan Hsieh freebsd_committer freebsd_triage 2020-10-09 12:03:16 UTC
Sorry, I thought I replied.

I prefer a patch file instead of doing it in configure stage. 
Could you please check this one [1]?
Thanks!

--- old/config.h
+++ new/config.h
@@ -126,7 +126,7 @@
 #define HAVE_STRNCASECMP 1

 /* Define to 1 if you have the `uselocale' function. */
-// #define HAVE_USELOCALE
+/* #undef HAVE_USELOCALE */

 /* Define to 1 if you have the `vasprintf' function. */
 #define HAVE_VASPRINTF

[1] https://people.FreeBSD.org/~sunpoet/patch/devel-json-c.txt
Comment 8 Craig Leres freebsd_committer freebsd_triage 2020-10-09 17:28:02 UTC
(In reply to Sunpoet Po-Chuan Hsieh from comment #7)
I guess I was thinking an advantage of making the change it in the port Makefile is it can be wrapped with:

    .if ${OPSYS} == FreeBSD && ${OSVERSION} >= 1203000
    ...
    .endif

or whatever after this is fixed in libc.

I tested your patch and it allows the test program to run without leaks.
Comment 9 Po-Chuan Hsieh freebsd_committer freebsd_triage 2020-10-09 19:03:33 UTC
(In reply to Craig Leres from comment #8)

I agree with you that it should be conditional when newlocale() is fixed. 

At that time, we could move the patch file to an extra patch (EXTRA_PATCHES) and wrap it with OSVERSION check.
Comment 10 commit-hook freebsd_committer freebsd_triage 2020-10-10 07:44:33 UTC
A commit references this bug:

Author: sunpoet
Date: Sat Oct 10 07:44:14 UTC 2020
New revision: 551865
URL: https://svnweb.freebsd.org/changeset/ports/551865

Log:
  Update r551672

  Use a patch file instead doing it in configure stage.

  PR:		249412

  Submitted by:	sunpoet (myself)
  Tested by:	leres

Changes:
  head/devel/json-c/Makefile
  head/devel/json-c/files/
  head/devel/json-c/files/patch-CMakeLists.txt