PYTHON-5013 Add NULL checks in InvalidDocument bson handling #2049
Conversation
bson/_cbsonmodule.c
Outdated
| @@ -1747,6 +1747,9 @@ int write_dict(PyObject* self, buffer_t buffer, | |||
| PyObject *etype = NULL, *evalue = NULL, *etrace = NULL; | |||
| PyErr_Fetch(&etype, &evalue, &etrace); | |||
| PyObject *InvalidDocument = _error("InvalidDocument"); | |||
| if (InvalidDocument == NULL) { | |||
| return 0; | |||
There was a problem hiding this comment.
This return case will leak &etype, &evalue, &etrace. We either need to call PyErr_Restore or xdecref those explicitly.
bson/_cbsonmodule.c
Outdated
| @@ -1760,6 +1763,9 @@ int write_dict(PyObject* self, buffer_t buffer, | |||
| if (msg) { | |||
| // Prepend doc to the existing message | |||
| PyObject *dict_str = PyObject_Str(dict); | |||
| if (dict_str == NULL) { | |||
| return 0; | |||
| } | |||
| PyObject *new_msg = PyUnicode_FromFormat("Invalid document %s | %s", PyUnicode_AsUTF8(dict_str), PyUnicode_AsUTF8(msg)); | |||
There was a problem hiding this comment.
Could you fix the indentation on line 1776?
There was a problem hiding this comment.
Also I believe PyUnicode_AsUTF8 can return NULL on error so we'll need checks there too. Unless there's a simpler API to construct this string that does the NULL checks for us.
bson/_cbsonmodule.c
Outdated
| @@ -1760,6 +1763,9 @@ int write_dict(PyObject* self, buffer_t buffer, | |||
| if (msg) { | |||
| // Prepend doc to the existing message | |||
| PyObject *dict_str = PyObject_Str(dict); | |||
| if (dict_str == NULL) { | |||
| return 0; | |||
There was a problem hiding this comment.
Same leak comment here except we'll also leak more variables, eg msg.
bson/_cbsonmodule.c
Outdated
| @@ -1747,6 +1747,9 @@ int write_dict(PyObject* self, buffer_t buffer, | |||
| PyObject *etype = NULL, *evalue = NULL, *etrace = NULL; | |||
| PyErr_Fetch(&etype, &evalue, &etrace); | |||
| PyObject *InvalidDocument = _error("InvalidDocument"); | |||
| if (InvalidDocument == NULL) { | |||
There was a problem hiding this comment.
I just realized there's a larger problem here. This logic is only in place for the is_dict fast path where it should be added generically for any mapping.
There was a problem hiding this comment.
Okay, I'll see if I can factor this out.
No description provided.