Skip to content

bpo-36389: Add _PyObject_CheckConsistency() function #12803

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 1 commit into from
Apr 12, 2019
Merged

bpo-36389: Add _PyObject_CheckConsistency() function #12803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions Include/cpython/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,21 @@ PyAPI_FUNC(void) _PyObject_AssertFailed(
int line,
const char *function);

/* Check if an object is consistent. For example, ensure that the reference
counter is greater than or equal to 1, and ensure that ob_type is not NULL.

Call _PyObject_AssertFailed() if the object is inconsistent.

If check_content is zero, only check header fields: reduce the overhead.

The function always return 1. The return value is just here to be able to
write:
Copy link
Member

Choose a reason for hiding this comment

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

Why would you write assert(_PyObject_CheckConsistency) if it always returns 1? Wouldn't it be better off being named AssertConsistent if it's going to do the assert itself?

It looks like the other functions like this return 1 or 0 and the caller does the assert.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would you write assert(_PyObject_CheckConsistency) if it always returns 1?

It's a practical solution to add checks in debug mode which are omitted in release mode. It's shorter than:

#ifndef NDEBUG
_PyObject_CheckConsistency();
#endif

or

#ifdef Py_DEBUG
_PyObject_CheckConsistency();
#endif

By the way, I recall that someone reported an issue when Py_DEBUG is build without assertions (with NDEBUG defined).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it, that makes sense. But if this is internal, why not just offer a macro that is defined to blank?

On Windows, Py_DEBUG is inferred from _DEBUG, which is the opposite of NDEBUG by default (but I'm not totally sure which one modifies assert). We should definitely use Py_DEBUG within the codebase for our own checks like this, and probably people should not set it directly? Or maybe we need to add some extra validation in PC/pyconfig.h to fail the build if it's inconsistent. (Let's worry about this when someone files a bug, rather than here :) )


assert(_PyObject_CheckConsistency(obj, 1)); */
PyAPI_FUNC(int) _PyObject_CheckConsistency(
PyObject *op,
int check_content);

#ifdef __cplusplus
}
#endif
4 changes: 4 additions & 0 deletions Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ extern "C" {

#include "pycore_pystate.h" /* _PyRuntime */

PyAPI_FUNC(int) _PyType_CheckConsistency(PyTypeObject *type);
PyAPI_FUNC(int) _PyUnicode_CheckConsistency(PyObject *op, int check_content);
PyAPI_FUNC(int) _PyDict_CheckConsistency(PyObject *mp, int check_content);
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a PyTuple_CheckConsistency that checks all elements are non-null and haven't been deallocated? We know there's a bug like this right now somewhere (though whether checking via the tuple would detect it or not I don't know...)


/* Tell the GC to track this object.
*
* NB: While the object is tracked by the collector, it must be safe to call the
Expand Down
118 changes: 59 additions & 59 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,77 +449,77 @@ static PyObject *empty_values[1] = { NULL };
/* Uncomment to check the dict content in _PyDict_CheckConsistency() */
/* #define DEBUG_PYDICT */

#ifdef DEBUG_PYDICT
# define ASSERT_CONSISTENT(op) assert(_PyDict_CheckConsistency((PyObject *)(op), 1))
#else
# define ASSERT_CONSISTENT(op) assert(_PyDict_CheckConsistency((PyObject *)(op), 0))
#endif

#ifndef NDEBUG
static int
_PyDict_CheckConsistency(PyDictObject *mp)

int
_PyDict_CheckConsistency(PyObject *op, int check_content)
{
#define ASSERT(expr) _PyObject_ASSERT((PyObject *)mp, (expr))
_PyObject_ASSERT(op, PyDict_Check(op));
PyDictObject *mp = (PyDictObject *)op;

PyDictKeysObject *keys = mp->ma_keys;
int splitted = _PyDict_HasSplitTable(mp);
Py_ssize_t usable = USABLE_FRACTION(keys->dk_size);
#ifdef DEBUG_PYDICT
PyDictKeyEntry *entries = DK_ENTRIES(keys);
Py_ssize_t i;
#endif

ASSERT(0 <= mp->ma_used && mp->ma_used <= usable);
ASSERT(IS_POWER_OF_2(keys->dk_size));
ASSERT(0 <= keys->dk_usable
&& keys->dk_usable <= usable);
ASSERT(0 <= keys->dk_nentries
&& keys->dk_nentries <= usable);
ASSERT(keys->dk_usable + keys->dk_nentries <= usable);
_PyObject_ASSERT(op, 0 <= mp->ma_used && mp->ma_used <= usable);
_PyObject_ASSERT(op, IS_POWER_OF_2(keys->dk_size));
_PyObject_ASSERT(op, 0 <= keys->dk_usable && keys->dk_usable <= usable);
_PyObject_ASSERT(op, 0 <= keys->dk_nentries && keys->dk_nentries <= usable);
_PyObject_ASSERT(op, keys->dk_usable + keys->dk_nentries <= usable);

if (!splitted) {
/* combined table */
ASSERT(keys->dk_refcnt == 1);
_PyObject_ASSERT(op, keys->dk_refcnt == 1);
}

#ifdef DEBUG_PYDICT
for (i=0; i < keys->dk_size; i++) {
Py_ssize_t ix = dictkeys_get_index(keys, i);
ASSERT(DKIX_DUMMY <= ix && ix <= usable);
}
if (check_content) {
PyDictKeyEntry *entries = DK_ENTRIES(keys);
Py_ssize_t i;

for (i=0; i < keys->dk_size; i++) {
Py_ssize_t ix = dictkeys_get_index(keys, i);
_PyObject_ASSERT(op, DKIX_DUMMY <= ix && ix <= usable);
}

for (i=0; i < usable; i++) {
PyDictKeyEntry *entry = &entries[i];
PyObject *key = entry->me_key;
for (i=0; i < usable; i++) {
PyDictKeyEntry *entry = &entries[i];
PyObject *key = entry->me_key;

if (key != NULL) {
if (PyUnicode_CheckExact(key)) {
Py_hash_t hash = ((PyASCIIObject *)key)->hash;
ASSERT(hash != -1);
ASSERT(entry->me_hash == hash);
}
else {
/* test_dict fails if PyObject_Hash() is called again */
ASSERT(entry->me_hash != -1);
if (key != NULL) {
if (PyUnicode_CheckExact(key)) {
Py_hash_t hash = ((PyASCIIObject *)key)->hash;
_PyObject_ASSERT(op, hash != -1);
_PyObject_ASSERT(op, entry->me_hash == hash);
}
else {
/* test_dict fails if PyObject_Hash() is called again */
_PyObject_ASSERT(op, entry->me_hash != -1);
}
if (!splitted) {
_PyObject_ASSERT(op, entry->me_value != NULL);
}
}
if (!splitted) {
ASSERT(entry->me_value != NULL);

if (splitted) {
_PyObject_ASSERT(op, entry->me_value == NULL);
}
}

if (splitted) {
ASSERT(entry->me_value == NULL);
/* splitted table */
for (i=0; i < mp->ma_used; i++) {
_PyObject_ASSERT(op, mp->ma_values[i] != NULL);
}
}
}

if (splitted) {
/* splitted table */
for (i=0; i < mp->ma_used; i++) {
ASSERT(mp->ma_values[i] != NULL);
}
}
#endif

return 1;

#undef ASSERT
}
#endif


static PyDictKeysObject *new_keys_object(Py_ssize_t size)
Expand Down Expand Up @@ -614,7 +614,7 @@ new_dict(PyDictKeysObject *keys, PyObject **values)
mp->ma_values = values;
mp->ma_used = 0;
mp->ma_version_tag = DICT_NEXT_VERSION();
assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
return (PyObject *)mp;
}

Expand Down Expand Up @@ -675,7 +675,7 @@ clone_combined_dict(PyDictObject *orig)
return NULL;
}
new->ma_used = orig->ma_used;
assert(_PyDict_CheckConsistency(new));
ASSERT_CONSISTENT(new);
if (_PyObject_GC_IS_TRACKED(orig)) {
/* Maintain tracking. */
_PyObject_GC_TRACK(new);
Expand Down Expand Up @@ -1075,7 +1075,7 @@ insertdict(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject *value)
mp->ma_keys->dk_usable--;
mp->ma_keys->dk_nentries++;
assert(mp->ma_keys->dk_usable >= 0);
assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
return 0;
}

Expand All @@ -1094,7 +1094,7 @@ insertdict(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject *value)

mp->ma_version_tag = DICT_NEXT_VERSION();
Py_XDECREF(old_value); /* which **CAN** re-enter (see issue #22653) */
assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
Py_DECREF(key);
return 0;

Expand Down Expand Up @@ -1582,7 +1582,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
Py_DECREF(old_key);
Py_DECREF(old_value);

assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
return 0;
}

Expand Down Expand Up @@ -1722,7 +1722,7 @@ PyDict_Clear(PyObject *op)
assert(oldkeys->dk_refcnt == 1);
dictkeys_decref(oldkeys);
}
assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
}

/* Internal version of PyDict_Next that returns a hash value in addition
Expand Down Expand Up @@ -1852,7 +1852,7 @@ _PyDict_Pop_KnownHash(PyObject *dict, PyObject *key, Py_hash_t hash, PyObject *d
ep->me_value = NULL;
Py_DECREF(old_key);

assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
return old_value;
}

Expand Down Expand Up @@ -2434,7 +2434,7 @@ PyDict_MergeFromSeq2(PyObject *d, PyObject *seq2, int override)
}

i = 0;
assert(_PyDict_CheckConsistency((PyDictObject *)d));
ASSERT_CONSISTENT(d);
goto Return;
Fail:
Py_XDECREF(item);
Expand Down Expand Up @@ -2586,7 +2586,7 @@ dict_merge(PyObject *a, PyObject *b, int override)
/* Iterator completed, via error */
return -1;
}
assert(_PyDict_CheckConsistency((PyDictObject *)a));
ASSERT_CONSISTENT(a);
return 0;
}

Expand Down Expand Up @@ -2950,7 +2950,7 @@ PyDict_SetDefault(PyObject *d, PyObject *key, PyObject *defaultobj)
mp->ma_version_tag = DICT_NEXT_VERSION();
}

assert(_PyDict_CheckConsistency(mp));
ASSERT_CONSISTENT(mp);
return value;
}

Expand Down Expand Up @@ -3069,7 +3069,7 @@ dict_popitem_impl(PyDictObject *self)
self->ma_keys->dk_nentries = i;
self->ma_used--;
self->ma_version_tag = DICT_NEXT_VERSION();
assert(_PyDict_CheckConsistency(self));
ASSERT_CONSISTENT(self);
return res;
}

Expand Down Expand Up @@ -3275,7 +3275,7 @@ dict_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
Py_DECREF(self);
return NULL;
}
assert(_PyDict_CheckConsistency(d));
ASSERT_CONSISTENT(d);
return self;
}

Expand Down
31 changes: 30 additions & 1 deletion Objects/object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
/* Generic object operations; and implementation of None */

#include "Python.h"
#include "pycore_object.h"
#include "pycore_pystate.h"
#include "pycore_context.h"
#include "frameobject.h"
Expand All @@ -19,6 +20,28 @@ _Py_IDENTIFIER(__bytes__);
_Py_IDENTIFIER(__dir__);
_Py_IDENTIFIER(__isabstractmethod__);


int
_PyObject_CheckConsistency(PyObject *op, int check_content)
{
_PyObject_ASSERT(op, op != NULL);
_PyObject_ASSERT(op, !_PyObject_IsFreed(op));
_PyObject_ASSERT(op, Py_REFCNT(op) >= 1);

PyTypeObject *type = op->ob_type;
_PyObject_ASSERT(op, type != NULL);
_PyType_CheckConsistency(type);

if (PyUnicode_Check(op)) {
_PyUnicode_CheckConsistency(op, check_content);
}
else if (PyDict_Check(op)) {
_PyDict_CheckConsistency(op, check_content);
}
return 1;
}


#ifdef Py_REF_DEBUG
Py_ssize_t _Py_RefTotal;

Expand Down Expand Up @@ -2136,7 +2159,13 @@ _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg,
else if (_PyObject_IsFreed(obj)) {
/* It seems like the object memory has been freed:
don't access it to prevent a segmentation fault. */
fprintf(stderr, "<Freed object>\n");
fprintf(stderr, "<object: freed>\n");
}
else if (Py_TYPE(obj) == NULL) {
fprintf(stderr, "<object: ob_type=NULL>\n");
}
else if (_PyObject_IsFreed((PyObject *)Py_TYPE(obj))) {
fprintf(stderr, "<object: freed type %p>\n", Py_TYPE(obj));
Copy link
Contributor

Choose a reason for hiding this comment

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

@vstinner If the argument for %p is not a pointer to void, the behavior is undefined. There needs to be a (void *) cast just before the Py_TYPE(obj) (see also GH-12769).

}
else {
/* Diplay the traceback where the object has been allocated.
Expand Down
11 changes: 6 additions & 5 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,7 @@ skip_signature(const char *doc)
return NULL;
}

#ifndef NDEBUG
static int
int
_PyType_CheckConsistency(PyTypeObject *type)
{
#define ASSERT(expr) _PyObject_ASSERT((PyObject *)type, (expr))
Expand All @@ -142,14 +141,16 @@ _PyType_CheckConsistency(PyTypeObject *type)
return 1;
}

ASSERT(!_PyObject_IsFreed((PyObject *)type));
ASSERT(Py_REFCNT(type) >= 1);
ASSERT(PyType_Check(type));

ASSERT(!(type->tp_flags & Py_TPFLAGS_READYING));
ASSERT(type->tp_mro != NULL && PyTuple_Check(type->tp_mro));
ASSERT(type->tp_dict != NULL);
return 1;

return 1;
#undef ASSERT
}
#endif

static const char *
_PyType_DocWithoutSignature(const char *name, const char *internal_doc)
Expand Down
Loading