Skip to content

Check dict item accesses where it isn't already checked #2863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 2, 2021
Merged

Check dict item accesses where it isn't already checked #2863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a47b70f
Convert PyDict_GetXXX to internal error checking variant
virtuald Feb 17, 2021
1eee738
Check unlikely error return from PyDict_DelItemString
virtuald Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,7 @@ class cpp_function : public function {
bool bad_arg = false;
for (; args_copied < args_to_copy; ++args_copied) {
const argument_record *arg_rec = args_copied < func.args.size() ? &func.args[args_copied] : nullptr;
if (kwargs_in && arg_rec && arg_rec->name && PyDict_GetItemString(kwargs_in, arg_rec->name)) {
if (kwargs_in && arg_rec && arg_rec->name && dict_getitemstring(kwargs_in, arg_rec->name)) {
bad_arg = true;
break;
}
Expand Down Expand Up @@ -684,15 +684,17 @@ class cpp_function : public function {

handle value;
if (kwargs_in && arg_rec.name)
value = PyDict_GetItemString(kwargs.ptr(), arg_rec.name);
value = dict_getitemstring(kwargs.ptr(), arg_rec.name);

if (value) {
// Consume a kwargs value
if (!copied_kwargs) {
kwargs = reinterpret_steal<dict>(PyDict_Copy(kwargs.ptr()));
copied_kwargs = true;
}
PyDict_DelItemString(kwargs.ptr(), arg_rec.name);
if (PyDict_DelItemString(kwargs.ptr(), arg_rec.name) == -1) {
throw error_already_set();
}
} else if (arg_rec.value) {
value = arg_rec.value;
}
Expand Down Expand Up @@ -2139,7 +2141,7 @@ inline function get_type_override(const void *this_ptr, const type_info *this_ty
if (frame && (std::string) str(frame->f_code->co_name) == name &&
frame->f_code->co_argcount > 0) {
PyFrame_FastToLocals(frame);
PyObject *self_caller = PyDict_GetItem(
PyObject *self_caller = dict_getitem(
frame->f_locals, PyTuple_GET_ITEM(frame->f_code->co_varnames, 0));
if (self_caller == self.ptr())
return function();
Expand Down
37 changes: 37 additions & 0 deletions include/pybind11/pytypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,43 @@ inline handle get_function(handle value) {
return value;
}

// Reimplementation of python's dict helper functions to ensure that exceptions
// aren't swallowed (see #2862)

// copied from cpython _PyDict_GetItemStringWithError
inline PyObject * dict_getitemstring(PyObject *v, const char *key)
{
#if PY_MAJOR_VERSION >= 3
PyObject *kv, *rv;
kv = PyUnicode_FromString(key);
if (kv == NULL) {
throw error_already_set();
}

rv = PyDict_GetItemWithError(v, kv);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, can't you just call dict_getitemstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, still has potential to raise an exception. See https://bugs.python.org/issue35459

As copied from cpython @ https://github.com/python/cpython/blob/d5fc99873769f0d0d5c5d5d99059177a75a4e46e/Objects/dictobject.c#L1545

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I was thinking about calling your dict_getitem. From this line on, dict_getitemstring and dict_getitem are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite the same, Py_DECREF needs to be called on the key before the error is thrown.

Py_DECREF(kv);
if (rv == NULL && PyErr_Occurred()) {
throw error_already_set();
}
return rv;
#else
return PyDict_GetItemString(v, key);
#endif
}

inline PyObject * dict_getitem(PyObject *v, PyObject *key)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this a member of py::dict?

Copy link
Contributor Author

@virtuald virtuald Feb 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the code that is intended to call this (function dispatcher) uses the raw CPython API, presumably for performance reasons?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

{
#if PY_MAJOR_VERSION >= 3
PyObject *rv = PyDict_GetItemWithError(v, key);
if (rv == NULL && PyErr_Occurred()) {
throw error_already_set();
}
return rv;
#else
return PyDict_GetItem(v, key);
#endif
}

// Helper aliases/functions to support implicit casting of values given to python accessors/methods.
// When given a pyobject, this simply returns the pyobject as-is; for other C++ type, the value goes
// through pybind11::cast(obj) to convert it to an `object`.
Expand Down