CLN validate some Py_* Functions in C Extensions check for NULL by MarcoGorelli · Pull Request #50997 · pandas-dev/pandas

MarcoGorelli · 2023-01-26T17:09:48Z

@WillAyd is this what you had in mind?

In cases like

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

Lines 902 to 906 in 3a1a8b3

    
           void Dir_iterBegin(JSOBJ obj, JSONTypeContext *tc) { 
        
               GET_TC(tc)->attrList = PyObject_Dir(obj); 
        
               GET_TC(tc)->index = 0; 
        
               GET_TC(tc)->size = PyList_GET_SIZE(GET_TC(tc)->attrList); 
        
           }

which return void, how should NULL be handled?

WillAyd

Cool thanks for trying this. Not easy

WillAyd · 2023-01-26T19:37:05Z


    func = PyObject_GetAttrString(src->obj, "read");
+    if (func == NULL) {
+        return NULL;


It looks like you also need to Py_DECREF(args) - looks to be Py_XDECREF a few lines down but won't happen if you return here.

As a side note - I think we also too liberally use Py_XDECREF. But that's another issue for another day

WillAyd · 2023-01-26T19:38:36Z

    if (decoder->curdim <= 0) {
        // start of array - initialise the context buffer
        npyarr = decoder->npyarr = PyObject_Malloc(sizeof(NpyArrContext));
+        if (npyarr == NULL) {


I think this is a bit redundant with L116, though I find the npyarr == NULL approach more explicit. I also think the position of this is better than L116 but need to combine these pieces of code

WillAyd · 2023-01-26T19:41:05Z

        npyarr->labels[0] = npyarr->labels[1] = NULL;

        npyarr->shape.ptr = PyObject_Malloc(sizeof(npy_intp) * NPY_MAXDIMS);
+        if (npyarr->shape.ptr == NULL) {


At this point in the code you have malloc'ed decoder->npyarr, so you also need to free it (using PyObject_Free instead of the built-in)

WillAyd · 2023-01-26T19:46:01Z


    *len = (size_t)get_datetime_iso_8601_strlen(0, base);
    char *result = PyObject_Malloc(*len);
+    if (result == NULL) {


I think the idiomatic approach here is to just do return PyErr_NoMemory()

https://docs.python.org/3/c-api/exceptions.html#c.PyErr_NoMemory

example from CPython source:

https://github.com/python/cpython/blob/9f2c479eaf7d922746ef2f3c85b5c781757686b1/Objects/unicodeobject.c#L1188

WillAyd · 2023-01-26T19:46:31Z


    pc = PyObject_Malloc(sizeof(TypeContext));
+    if (pc == NULL) {
+        return NULL;


Same comment here about PyErr_NoMemory

Though again this is the same as L155, though I think the pc == NULL check is a lot more explicit

WillAyd · 2023-01-26T19:49:01Z

                // be lost if values is used directly.
                Py_DECREF(tz);
                values = PyObject_CallMethod(obj, "__array__", NULL);
+                if (values == NULL) {


I'm +/-0 on having to do this here. If values is NULL the return values naturally propagates up the caller, who is also responsible for checking. But if it helps enforce patterns then this is harmless

WillAyd · 2023-01-26T19:50:18Z

        if (PyObject_HasAttrString(obj, "dtype")) {
            PyObject *dtype = PyObject_GetAttrString(obj, "dtype");
+            if (dtype == NULL) {
+                return NULL;


Need to Py_DECREF(typeRepr) before returning

WillAyd · 2023-01-26T19:52:19Z

+            }
            repr = PyObject_Repr(dtype);
+            if (repr == NULL) {
+                return NULL;


Same comment above on Py_DECREF(typeRepr); might be a clever way to refactor the function so you don't need to repeat in every error handler, but also OK to repeat for now

WillAyd · 2023-01-26T19:53:20Z

+    if (ret == NULL) {
+        return ret;
+    }
    Py_DECREF(tmp);


I guess this is a case where it could make sense to have say Py_DECREF right before the error checking; whether or not things worked you are down with the lifecycle of tmp

WillAyd · 2023-01-26T19:57:22Z

For functions that return void your hands are a bit tied. In that particular case PyObject_Dir should be setting the global python indicator, so somewhere along the lines you could do a PyErr_Occurred() check and see what happened. But that's thorny so I would just ignore for now.

Would be curious how that gets handled upstream in ultrajson. Seems like there's a much more natural way to serialize the built-in types than what we have here

github-actions · 2023-03-27T00:05:46Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

MarcoGorelli · 2023-03-27T08:48:02Z

thanks for your review

I probably won't have a chance to update this any time soon due to there being quite a few other tasks to take care of, so closing for now to clear the queue

wip

3a1a8b3

MarcoGorelli requested a review from WillAyd January 26, 2023 17:09

WillAyd requested changes Jan 26, 2023

View reviewed changes

simonjayhawkins added the Clean label Feb 22, 2023

github-actions Bot added the Stale label Mar 27, 2023

MarcoGorelli closed this Mar 27, 2023

	void Dir_iterBegin(JSOBJ obj, JSONTypeContext *tc) {
	GET_TC(tc)->attrList = PyObject_Dir(obj);
	GET_TC(tc)->index = 0;
	GET_TC(tc)->size = PyList_GET_SIZE(GET_TC(tc)->attrList);
	}

Uh oh!

Uh oh!

Conversation

MarcoGorelli commented Jan 26, 2023

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 Jan 26, 2023

Uh oh!

github-actions Bot commented Mar 27, 2023

Uh oh!

MarcoGorelli commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants