Skip to content
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.

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

PyStructSequence does not handle exceptions correctly #62669

Closed
vstinner opened this issue Jul 16, 2013 · 3 comments
Closed

PyStructSequence does not handle exceptions correctly #62669

vstinner opened this issue Jul 16, 2013 · 3 comments

Comments

@vstinner
Copy link
Member

BPO 18469
Nosy @vstinner, @serhiy-storchaka

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-07-16.23:24:36.603>
created_at = <Date 2013-07-16.00:32:30.269>
labels = []
title = 'PyStructSequence does not handle exceptions correctly'
updated_at = <Date 2013-07-16.23:24:36.601>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2013-07-16.23:24:36.601>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2013-07-16.23:24:36.603>
closer = 'python-dev'
components = []
creation = <Date 2013-07-16.00:32:30.269>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 18469
keywords = []
message_count = 3.0
messages = ['193139', '193141', '193203']
nosy_count = 3.0
nosy_names = ['vstinner', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue18469'
versions = ['Python 3.4']

@vstinner
Copy link
Member Author

Using pyfailmalloc (*) (see issue bpo-18408), I found bugs in the PyStructSequence API.

The following macros in Objects/structseq.c does not check for exceptions:

#define VISIBLE_SIZE(op) Py_SIZE(op)
#define VISIBLE_SIZE_TP(tp) PyLong_AsLong( \
                      PyDict_GetItemString((tp)->tp_dict, visible_length_key))

#define REAL_SIZE_TP(tp) PyLong_AsLong( \
                      PyDict_GetItemString((tp)->tp_dict, real_length_key))
#define REAL_SIZE(op) REAL_SIZE_TP(Py_TYPE(op))

#define UNNAMED_FIELDS_TP(tp) PyLong_AsLong( \
                      PyDict_GetItemString((tp)->tp_dict, unnamed_fields_key))
#define UNNAMED_FIELDS(op) UNNAMED_FIELDS_TP(Py_TYPE(op))

Exceptions in PyDict_GetItemString() and PyLong_AsLong() are "unlikely" (the request key always exist, except in a newly developed module, which is not the case here), but become very likely using pyfailmalloc: PyDict_GetItemString() allocates a temporary string, and the memory allocation can fail.

In my opinion, the PyStructSequence structure should store the number of visible, real and unnamed fields. The problem is that it would require a design of the structure: data cannot be added between tuple items and the tuple header. PyStructSequence is currently defined as:

typedef PyTupleObject PyStructSequence;

Another option is to detect and handle correctly exceptions where these macros are used. But how should we handle exceptions in structseq_dealloc() ???

static void
structseq_dealloc(PyStructSequence *obj)
{
    Py_ssize_t i, size;
    
    size = REAL_SIZE(obj);
    for (i = 0; i < size; ++i) {
        Py_XDECREF(obj->ob_item[i]);
    }
    PyObject_GC_Del(obj);
}

By the way, structseq_dealloc() might restore the original size ("Py_SIZE(obj) = REAL_SIZE(obj);") before calling the tuple destructor (even if tupledealloc() only keeps real tuple in its free list).

(*) https://pypi.python.org/pypi/pyfailmalloc

@vstinner
Copy link
Member Author

This issue is common in test_datetime and test_import tests using failmalloc, especially if _PyErr_BadInternalCall() fails immedialy with an assertion error.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Jul 16, 2013

New changeset af18829a7754 by Victor Stinner in branch 'default':
Close bpo-18469: Replace PyDict_GetItemString() with _PyDict_GetItemId() in structseq.c
http://hg.python.org/cpython/rev/af18829a7754

@python-dev python-dev mannequin closed this as completed Jul 16, 2013
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant