BUG: Fix unintialized strlen when PyUnicode_AsUTF8AndSize fails
State: closed

- [ ] xref #50324 (Replace xxxx with the GitHub issue number)
- [ ] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [ ] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [ ] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) 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. 


Labels:
name: Bug
name: IO JSON
description: read_json, to_json, json_normalize

Comments:
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

> 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
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!
> 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?
> > 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).
Might be worth trying -fanalyzer with gcc

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

> 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.
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