davidhewitt / pythonize

MIT License
207 stars 28 forks source link

x86 Segmentation fault on depythonize #78

Open keats720 opened 1 day ago

keats720 commented 1 day ago

Ran into an issue with x86 where heavily nested data was causing a segmentation fault on depythonize.

Issue came from using sqloxide to get ast for queries, narrowed it down to be happening in depythonize. The implementation works with no issue on Arm architecture. Not familiar with rust so I am not sure exactly how to get a full stack trace on the error, but thought it would be good to try and give some information.

The python dictionary has a depth close to 1000, it is an AST of a query with ~800 unions and is represented by rust sqlparser crate as {left: <>, right: {left: <>, right: ...}}.

I am not sure if it is something small or a complex issue, happy to provide more information if you can guide me on how

MarshalX commented 1 day ago

You are right about depth. The deserialization is recursive, and there is no control of the depth at all. You get a segmentation fault because of stack overflow. The limit depends on the machine configuration. That's why it could pass in the environment with an ARM arch.

Let's take a look at map deserialization: https://github.com/davidhewitt/pythonize/blob/4491cdb46f35ab3fc1b419beef92171fbef510da/src/de.rs#L286-L291

As we can see there is no depth counting or calling Py_EnterRecursiveCall + Py_LeaveRecursiveCall.

Here is the example of how serde json fixed the same issue: https://github.com/serde-rs/json/pull/163/files (tl;dr count remaining_depth; hard limit to 128; if reach return error; do -1 before visit_map and +1 after visit_map)

Speaking of the Py_EnterRecursiveCall + Py_LeaveRecursiveCall I used it in the exact same bug report in my lib. This is the best and smoothest option for the devs, but, from my testing, it gives 30% perf degradation. You can check it here: https://github.com/MarshalX/python-libipld/pull/51

@davidhewitt I will be happy to read your thoughts!