Skip to content

Commit

Permalink
bpo-42006: Stop using PyDict_GetItem, PyDict_GetItemString and _PyDic…
Browse files Browse the repository at this point in the history
…t_GetItemId.

These functions are considered not safe because they suppress all internal errors
and can return wrong result.  PyDict_GetItemString and _PyDict_GetItemId can
also silence current exception in rare cases.
  • Loading branch information
serhiy-storchaka committed Oct 11, 2020
1 parent 8287aad commit cd49ec3
Show file tree
Hide file tree
Showing 16 changed files with 241 additions and 125 deletions.
1 change: 0 additions & 1 deletion Include/cpython/dictobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ PyObject *_PyDict_FromKeys(PyObject *, PyObject *, PyObject *);
argument is raised.
*/
PyAPI_FUNC(int) _PyDict_MergeEx(PyObject *mp, PyObject *other, int override);
PyAPI_FUNC(PyObject *) _PyDict_GetItemId(PyObject *dp, struct _Py_Identifier *key);
PyAPI_FUNC(int) _PyDict_SetItemId(PyObject *dp, struct _Py_Identifier *key, PyObject *item);

PyAPI_FUNC(int) _PyDict_DelItemId(PyObject *mp, struct _Py_Identifier *key);
Expand Down
47 changes: 30 additions & 17 deletions Modules/_decimal/_decimal.c
Original file line number Diff line number Diff line change
Expand Up @@ -3186,6 +3186,31 @@ dotsep_as_utf8(const char *s)
return utf8;
}

static int
dict_get_item_string(PyObject *dict, const char *key, PyObject **valueobj, const char **valuestr)
{
*valueobj = NULL;
PyObject *keyobj = PyUnicode_FromString(key);
if (keyobj == NULL) {
return -1;
}
PyObject *value = PyDict_GetItemWithError(dict, keyobj);
Py_DECREF(keyobj);
if (value == NULL) {
if (PyErr_Occurred()) {
return -1;
}
return 0;
}
value = PyUnicode_AsUTF8String(value);
if (value == NULL) {
return -1;
}
*valueobj = value;
*valuestr = PyBytes_AS_STRING(value);
return 0;
}

/* Formatted representation of a PyDecObject. */
static PyObject *
dec_format(PyObject *dec, PyObject *args)
Expand Down Expand Up @@ -3256,23 +3281,11 @@ dec_format(PyObject *dec, PyObject *args)
"optional argument must be a dict");
goto finish;
}
if ((dot = PyDict_GetItemString(override, "decimal_point"))) {
if ((dot = PyUnicode_AsUTF8String(dot)) == NULL) {
goto finish;
}
spec.dot = PyBytes_AS_STRING(dot);
}
if ((sep = PyDict_GetItemString(override, "thousands_sep"))) {
if ((sep = PyUnicode_AsUTF8String(sep)) == NULL) {
goto finish;
}
spec.sep = PyBytes_AS_STRING(sep);
}
if ((grouping = PyDict_GetItemString(override, "grouping"))) {
if ((grouping = PyUnicode_AsUTF8String(grouping)) == NULL) {
goto finish;
}
spec.grouping = PyBytes_AS_STRING(grouping);
if (dict_get_item_string(override, "decimal_point", &dot, &spec.dot) ||
dict_get_item_string(override, "thousands_sep", &sep, &spec.sep) ||
dict_get_item_string(override, "grouping", &grouping, &spec.grouping))
{
goto finish;
}
if (mpd_validate_lconv(&spec) < 0) {
PyErr_SetString(PyExc_ValueError,
Expand Down
8 changes: 6 additions & 2 deletions Modules/_threadmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -816,10 +816,14 @@ local_clear(localobject *self)
for(tstate = PyInterpreterState_ThreadHead(tstate->interp);
tstate;
tstate = PyThreadState_Next(tstate))
if (tstate->dict && PyDict_GetItem(tstate->dict, self->key)) {
if (PyDict_DelItem(tstate->dict, self->key)) {
if (tstate->dict) {
PyObject *v = _PyDict_Pop(tstate->dict, self->key, Py_None);
if (v == NULL) {
PyErr_Clear();
}
else {
Py_DECREF(v);
}
}
}
return 0;
Expand Down
18 changes: 7 additions & 11 deletions Modules/_zoneinfo.c
Original file line number Diff line number Diff line change
Expand Up @@ -722,17 +722,16 @@ zoneinfo__unpickle(PyTypeObject *cls, PyObject *args)
static PyObject *
load_timedelta(long seconds)
{
PyObject *rv = NULL;
PyObject *rv;
PyObject *pyoffset = PyLong_FromLong(seconds);
if (pyoffset == NULL) {
return NULL;
}
int contains = PyDict_Contains(TIMEDELTA_CACHE, pyoffset);
if (contains == -1) {
goto error;
}

if (!contains) {
rv = PyDict_GetItemWithError(TIMEDELTA_CACHE, pyoffset);
if (rv == NULL) {
if (PyErr_Occurred()) {
goto error;
}
PyObject *tmp = PyDateTimeAPI->Delta_FromDelta(
0, seconds, 0, 1, PyDateTimeAPI->DeltaType);

Expand All @@ -743,12 +742,9 @@ load_timedelta(long seconds)
rv = PyDict_SetDefault(TIMEDELTA_CACHE, pyoffset, tmp);
Py_DECREF(tmp);
}
else {
rv = PyDict_GetItem(TIMEDELTA_CACHE, pyoffset);
}

Py_XINCREF(rv);
Py_DECREF(pyoffset);
Py_INCREF(rv);
return rv;
error:
Py_DECREF(pyoffset);
Expand Down
3 changes: 1 addition & 2 deletions Modules/signalmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1427,10 +1427,9 @@ signal_exec(PyObject *m)
return -1;
#endif

IntHandler = PyDict_GetItemString(d, "default_int_handler");
IntHandler = PyMapping_GetItemString(d, "default_int_handler");
if (!IntHandler)
return -1;
Py_INCREF(IntHandler);

_Py_atomic_store_relaxed(&Handlers[0].tripped, 0);
for (int i = 1; i < NSIG; i++) {
Expand Down
31 changes: 18 additions & 13 deletions Modules/socketmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ static FlagRuntimeInfo win_runtime_flags[] = {
{14393, "TCP_FASTOPEN"}
};

static void
static int
remove_unusable_flags(PyObject *m)
{
PyObject *dict;
Expand All @@ -333,7 +333,7 @@ remove_unusable_flags(PyObject *m)

dict = PyModule_GetDict(m);
if (dict == NULL) {
return;
return 0;
}

/* set to Windows 10, except BuildNumber. */
Expand All @@ -359,19 +359,21 @@ remove_unusable_flags(PyObject *m)
break;
}
else {
if (PyDict_GetItemString(
dict,
win_runtime_flags[i].flag_name) != NULL)
{
if (PyDict_DelItemString(
dict,
win_runtime_flags[i].flag_name))
{
PyErr_Clear();
}
PyObject *flag_name = PyUnicode_FromString(win_runtime_flags[i].flag_name);
if (flag_name == NULL) {
return -1;
}
PyObject *v = _PyDict_Pop(dict, flag_name, Py_None);
Py_DECREF(flag_name);
if (v != NULL) {
Py_DECREF(v);
}
else if (PyErr_Occurred()) {
return -1;
}
}
}
return 0;
}

#endif
Expand Down Expand Up @@ -8382,7 +8384,10 @@ PyInit__socket(void)

#ifdef MS_WINDOWS
/* remove some flags on older version Windows during run-time */
remove_unusable_flags(m);
if (remove_unusable_flags(m) < 0) {
Py_DECREF(m);
return NULL;
}
#endif

return m;
Expand Down
12 changes: 0 additions & 12 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3525,18 +3525,6 @@ PyTypeObject PyDict_Type = {
.tp_vectorcall = dict_vectorcall,
};

PyObject *
_PyDict_GetItemId(PyObject *dp, struct _Py_Identifier *key)
{
PyObject *kv;
kv = _PyUnicode_FromId(key); /* borrowed */
if (kv == NULL) {
PyErr_Clear();
return NULL;
}
return PyDict_GetItem(dp, kv);
}

/* For backward compatibility with old dictionary interface */

PyObject *
Expand Down
28 changes: 21 additions & 7 deletions Objects/moduleobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -477,10 +477,12 @@ PyModule_GetNameObject(PyObject *m)
}
d = ((PyModuleObject *)m)->md_dict;
if (d == NULL ||
(name = _PyDict_GetItemId(d, &PyId___name__)) == NULL ||
(name = _PyDict_GetItemIdWithError(d, &PyId___name__)) == NULL ||
!PyUnicode_Check(name))
{
PyErr_SetString(PyExc_SystemError, "nameless module");
if (!PyErr_Occurred()) {
PyErr_SetString(PyExc_SystemError, "nameless module");
}
return NULL;
}
Py_INCREF(name);
Expand Down Expand Up @@ -509,10 +511,12 @@ PyModule_GetFilenameObject(PyObject *m)
}
d = ((PyModuleObject *)m)->md_dict;
if (d == NULL ||
(fileobj = _PyDict_GetItemId(d, &PyId___file__)) == NULL ||
(fileobj = _PyDict_GetItemIdWithError(d, &PyId___file__)) == NULL ||
!PyUnicode_Check(fileobj))
{
PyErr_SetString(PyExc_SystemError, "module filename missing");
if (!PyErr_Occurred()) {
PyErr_SetString(PyExc_SystemError, "module filename missing");
}
return NULL;
}
Py_INCREF(fileobj);
Expand Down Expand Up @@ -721,14 +725,21 @@ module_getattro(PyModuleObject *m, PyObject *name)
PyErr_Clear();
if (m->md_dict) {
_Py_IDENTIFIER(__getattr__);
getattr = _PyDict_GetItemId(m->md_dict, &PyId___getattr__);
getattr = _PyDict_GetItemIdWithError(m->md_dict, &PyId___getattr__);
if (getattr) {
return PyObject_CallOneArg(getattr, name);
}
mod_name = _PyDict_GetItemId(m->md_dict, &PyId___name__);
if (PyErr_Occurred()) {
return NULL;
}
mod_name = _PyDict_GetItemIdWithError(m->md_dict, &PyId___name__);
if (mod_name && PyUnicode_Check(mod_name)) {
Py_INCREF(mod_name);
PyObject *spec = _PyDict_GetItemId(m->md_dict, &PyId___spec__);
PyObject *spec = _PyDict_GetItemIdWithError(m->md_dict, &PyId___spec__);
if (spec == NULL && PyErr_Occurred()) {
Py_DECREF(mod_name);
return NULL;
}
Py_XINCREF(spec);
if (_PyModuleSpec_IsInitializing(spec)) {
PyErr_Format(PyExc_AttributeError,
Expand All @@ -746,6 +757,9 @@ module_getattro(PyModuleObject *m, PyObject *name)
Py_DECREF(mod_name);
return NULL;
}
else if (PyErr_Occurred()) {
return NULL;
}
}
PyErr_Format(PyExc_AttributeError,
"module has no attribute '%U'", name);
Expand Down
Loading

0 comments on commit cd49ec3

Please sign in to comment.