BUG: fix memory leak in JSON datetime serialization #62217

Alvaro-Kothe · 2025-08-29T18:15:32Z

closes BUG: memory leak in to_json when converting DateTime values #62204 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

swt2c · 2025-08-29T18:52:15Z

Thanks @Alvaro-Kothe, I can confirm this resolves the memory leak that I am seeing. Also, your solution makes sense.

👍

swt2c

I think you might also need to add a GET_TC(tc)->freeCStr = 1; in Object_getBigNumStringValue

Alvaro-Kothe · 2025-08-29T21:57:52Z

Thanks for pointing that out. Just fixed it.

WillAyd · 2025-09-02T13:18:14Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

@@ -107,6 +107,7 @@ typedef struct __TypeContext {
  JSINT64 longValue;

  const char *cStr;
+  int freeCStr;


Generally the problem with the JSON code is that over time we have incrementally added many different state management operations, and it is not always clear how they should work in tandem. So I am a little hesitant to add more state to solve the current issue. Is it possible to solve the issue without changing this struct?

Any suggestions on how to do that? Revert the changes in fb6c4e3 that make some of the uses of cStr stack allocated? It doesn't seem possible to make all uses of cStr stack allocated, at least at first glance.

Sure that might be a reasonable approach

I removed the freeCStr flag and stored all the strings assigned in *IterNext in dynamic memory.

Revert "fix: flag to free `cStr` in `Object_getBigNumStringValue`" Revert "fix: memory leak in JSON datetime serialization" fix: fix memory leak in `PyDateTimeToIsoCallback` fix: ensure successful malloc.

WillAyd · 2025-09-02T16:17:55Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

@@ -1880,6 +1905,7 @@ static void Object_endTypeContext(JSOBJ Py_UNUSED(obj), JSONTypeContext *tc) {
    GET_TC(tc)->rowLabels = NULL;
    NpyArr_freeLabels(GET_TC(tc)->columnLabels, GET_TC(tc)->columnLabelsLen);
    GET_TC(tc)->columnLabels = NULL;
+    PyObject_Free((void *)GET_TC(tc)->cStr);


I'm not too sure about this approach - generally the CPython API only wants you to call this function when you've allocated with the corresponding function(s)

https://docs.python.org/3/c-api/memory.html#c.PyObject_Free

Frees the memory block pointed to by p, which must have been returned by a previous call to PyObject_Malloc(), PyObject_Realloc() or PyObject_Calloc(). Otherwise, or if PyObject_Free(p) has been called before, undefined behavior occurs.

Ah sorry I misread your PR. I see you allocate 20 bytes to cStr, so this is clean in that regards. However, I think you want to use the PyMem_ functions since we are dealing with raw C constructs and not Python objects, so maybe replace this with PyMem_Free, any PyObject_Malloc calls with PyMem_Malloc

Unless I'm missing something, PyObject_Free() is being called on memory that's been allocated with PyObject_Malloc()...

WillAyd

Are there any object types where it is possible to go through the destructor without initializing the cStr member? Looking at say the dict iteration code it seems like that may be happening?

Its a bit confusing here that the type context begin functions have an implicit requirement to allocate the cStr member. I wonder if it wouldn't be cleaner to allocate that up front, and leave it to individual functions to free or replace memory as required

WillAyd · 2025-09-02T16:29:12Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

@@ -1007,16 +1008,24 @@ static const char *List_iterGetName(JSOBJ Py_UNUSED(obj),
 //=============================================================================
 static void Index_iterBegin(JSOBJ Py_UNUSED(obj), JSONTypeContext *tc) {
  GET_TC(tc)->index = 0;
+  GET_TC(tc)->cStr = PyObject_Malloc(20);


Its probably better to create a static const variable for a size within the module, rather than repeating the hard-coded 20 multiple times

I created the variable CSTR_SIZE

WillAyd · 2025-09-02T16:31:41Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

  if (index == 0) {
-    GET_TC(tc)->cStr = "name";
+    strcpy((char *)GET_TC(tc)->cStr, "name");


I think if you want to use strcpy here you will need to drop the const qualifier on the struct member; casting away const-ness like this is going to trigger undefined behavior

I removed the const qualifier.

WillAyd · 2025-09-02T16:37:08Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

@@ -1880,6 +1905,7 @@ static void Object_endTypeContext(JSOBJ Py_UNUSED(obj), JSONTypeContext *tc) {
    GET_TC(tc)->rowLabels = NULL;
    NpyArr_freeLabels(GET_TC(tc)->columnLabels, GET_TC(tc)->columnLabelsLen);
    GET_TC(tc)->columnLabels = NULL;
+    PyObject_Free((void *)GET_TC(tc)->cStr);


Why cast to a void pointer here?

It was because of the const qualifier in cStr, but since I removed this qualifier, I am no longer casting.

WillAyd · 2025-09-02T21:10:19Z

pandas/_libs/src/datetime/date_conversions.c

@@ -49,7 +49,7 @@ char *int64ToIso(int64_t value, NPY_DATETIMEUNIT valueUnit,
  pandas_datetime_to_datetimestruct(value, valueUnit, &dts);

  *len = (size_t)get_datetime_iso_8601_strlen(0, base);
-  char *result = PyObject_Malloc(*len);
+  char *result = PyMem_Malloc(*len);


Since the focus of the PR is to fix the JSON modules, let's leave the others untouched for now. These may be viable fixes, but they can always be done in a follow up

Ah nevermind - I guess you are doing this because the JSON module makes use of this function

Exacly, I made this change because of the assignment of cStr in NpyDateTimeToIsoCallback.

I also made changes in other functions to allocate with PyMem_Malloc in int64ToIsoDuration and PyDateTimeToIso, but I think that the CI errors are because of this changes. Is it correct to use PyMem_Malloc in them or should I change back to PyObject_Malloc?

Strange - I don't think PyObject_Malloc is correct. Upstream, it looks like they have made a similar change (see python/cpython#114569)

However, if its a blocker for now sure lets stick with the PyObject_ functions; can figure out the rest later

WillAyd · 2025-09-02T21:26:40Z

@Alvaro-Kothe the address and undefined behavior violations can be pretty tricky to debug. If you aren't aware, you can turn on sanitizers locally to better understand what is happening (assuming your compiler supports them):

python -m pip install -e . -Csetup-args="-Db_sanitize=address,undefined" -Csetupargs="-Dbuildtype=debug"

WillAyd

Cool nice work

WillAyd · 2025-09-02T23:09:18Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

@@ -1880,6 +1907,7 @@ static void Object_endTypeContext(JSOBJ Py_UNUSED(obj), JSONTypeContext *tc) {
    GET_TC(tc)->rowLabels = NULL;
    NpyArr_freeLabels(GET_TC(tc)->columnLabels, GET_TC(tc)->columnLabelsLen);
    GET_TC(tc)->columnLabels = NULL;
+    PyObject_Free(GET_TC(tc)->cStr);


Can you double check the paths that actually hit this? Right now it seems like we either implicitly have some kind of relation with the types allocate storage for the struct member and this function getting called, or we have undefined behavior when always calling free here, if the type doesn't allocate this in the first place

I checked these paths where cStr is assigned to a dynamic memory allocated with PyObject_Malloc:

NpyDateTimeToIsoCallback -> int64ToIso

NpyTimeDeltaToIsoCallback -> int64ToIsoDuration

PyDateTimeToIsoCallback -> PyDateTimeToIso

Index_iterBegin

Series_iterBegin

DataFrame_iterBegin

Object_getBigNumStringValue

There is no *_Free call for cStr outside of the Object_endTypeContext function to cause an undefined behaviour.

Other cases seems like cStr isn't assigned and end up being NULL, making the PyObject_Free a no-op.

WillAyd · 2025-09-03T00:45:43Z

Great thanks @Alvaro-Kothe ! Definitely open to any other improvements you can find here

fix: memory leak in JSON datetime serialization

791bb41

Alvaro-Kothe mentioned this pull request Aug 29, 2025

BUG: memory leak in to_json when converting DateTime values #62204

Closed

3 tasks

mroeschke requested a review from WillAyd August 29, 2025 19:02

Alvaro-Kothe marked this pull request as ready for review August 29, 2025 19:04

swt2c reviewed Aug 29, 2025

View reviewed changes

fix: flag to free cStr in Object_getBigNumStringValue

f7b2f5b

WillAyd reviewed Sep 2, 2025

View reviewed changes

fix: use dynamic allocated memory to store strings

f324bd8

Revert "fix: flag to free `cStr` in `Object_getBigNumStringValue`" Revert "fix: memory leak in JSON datetime serialization" fix: fix memory leak in `PyDateTimeToIsoCallback` fix: ensure successful malloc.

WillAyd reviewed Sep 2, 2025

View reviewed changes

WillAyd requested changes Sep 2, 2025

View reviewed changes

Alvaro-Kothe added 5 commits September 2, 2025 13:54

fix: replace PyObject_Malloc with PyMem_Malloc

15c19c0

fix: drop const qualifier in cStr

1ea0283

fix: remove void pointer case in PyMem_Free

0925e6b

fix: create const for default cStr size

62b0e26

try to fix ci error

0f20b7d

WillAyd reviewed Sep 2, 2025

View reviewed changes

hack: go back to use PyObject_* because of CI errors

05f4926

WillAyd reviewed Sep 2, 2025

View reviewed changes

WillAyd approved these changes Sep 3, 2025

View reviewed changes

WillAyd merged commit b9da662 into pandas-dev:main Sep 3, 2025
38 checks passed

Alvaro-Kothe deleted the fix/memleak-json-iso branch September 3, 2025 00:47

Uh oh!

BUG: fix memory leak in JSON datetime serialization #62217

BUG: fix memory leak in JSON datetime serialization #62217

Conversation

Alvaro-Kothe commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swt2c commented Aug 29, 2025

Uh oh!

swt2c left a comment

Choose a reason for hiding this comment

Uh oh!

Alvaro-Kothe commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swt2c Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Sep 2, 2025

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WillAyd commented Sep 3, 2025

Uh oh!

Uh oh!

Alvaro-Kothe commented Aug 29, 2025 •

edited

Loading

swt2c Sep 2, 2025 •

edited

Loading