BUG: Fix unintialized strlen when PyUnicode_AsUTF8AndSize fails

[lithomas1]

• xref BUG: to_json segfaults when exception occurs in UTF8 encoding of string #50324 (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.

Hopefully fixes 32bit builds.

[WillAyd]

Very nice find. Makes sense as far as the issue goes, but ideally we'd work with the Python C API pattern and leave as is. Do you think it's just a limitation of our ujson that prevents that? Wonder if there's an upstream fix there we should look at

[lithomas1]

Very nice find. Makes sense as far as the issue goes, but ideally we'd work with the Python C API pattern and leave as is. Do you think it's just a limitation of our ujson that prevents that? Wonder if there's an upstream fix there we should look at

We are allocating a buffer for the string, before checking that getStringValue succeeded.

https://github.com/lithomas1/pandas/blob/c279d921b2ecac840943c0781f813ad88c5da942/pandas/_libs/src/ujson/lib/ultrajsonenc.c#L1082-L1087

That's probably a good thing to follow up on.

ujson does this correctly, though. See
https://github.com/ultrajson/ultrajson/blob/1876c02e0fd765895670d04591422202950a4358/lib/ultrajsonenc.c#L882-L889

[lithomas1]

I'll also look into setting up valgrind/asan for our C code to catch stuff like this in the future(also nice for preventing memleaks/security headaches down the road).

Thanks everyone for helping me to debug this!

[WillAyd]

ujson does this correctly, though. See https://github.com/ultrajson/ultrajson/blob/1876c02e0fd765895670d04591422202950a4358/lib/ultrajsonenc.c#L882-L889

Nice find. I would rather vendor this - or is that causing issues?

[lithomas1]

ujson does this correctly, though. See https://github.com/ultrajson/ultrajson/blob/1876c02e0fd765895670d04591422202950a4358/lib/ultrajsonenc.c#L882-L889

Nice find. I would rather vendor this - or is that causing issues?

It should be fine, I think. I'd appreciate more scrutiny over this though (worried about double free/memleaks with the buffer here).

There are also more usages elsewhere to change(which is probably a nice followup).

[WillAyd]

Might be worth trying -fanalyzer with gcc

https://gcc.gnu.org/onlinedocs/gcc/Static-Analyzer-Options.html

[lithomas1]

Might be worth trying -fanalyzer with gcc

https://gcc.gnu.org/onlinedocs/gcc/Static-Analyzer-Options.html

No warnings from the json module at least.
Note: The C parser is throwing some warnings, though, and compiling the c++ extension modules crashes gcc(maybe it's oom-ing on 32bit).

I've also left in the initialization of the string length just to be safe though.

[WillAyd]

We should remove the 0 string length - at the end of the day we have NULL so the length is undefined. Want to stick with CPython standards too

For the gcc crash, is it gcc that is crashing or g++? I'm not sure the history of why that one module in particular uses c++ - could try simplifying and sticking with c consistently

[WillAyd]

lgtm when green