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

[C API] Enforce usage of PyFrame_GetBack() #90514

Closed
vstinner opened this issue Jan 12, 2022 · 7 comments
Closed

[C API] Enforce usage of PyFrame_GetBack() #90514

vstinner opened this issue Jan 12, 2022 · 7 comments
Labels
3.11 only security fixes topic-C-API

Comments

@vstinner
Copy link
Member

BPO 46356
Nosy @vstinner, @markshannon, @corona10

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 2022-02-25.15:00:16.682>
created_at = <Date 2022-01-12.15:33:01.875>
labels = ['expert-C-API', '3.11']
title = '[C API] Enforce usage of PyFrame_GetBack()'
updated_at = <Date 2022-02-25.15:00:16.681>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2022-02-25.15:00:16.681>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2022-02-25.15:00:16.682>
closer = 'vstinner'
components = ['C API']
creation = <Date 2022-01-12.15:33:01.875>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 46356
keywords = []
message_count = 7.0
messages = ['410403', '410405', '410406', '410408', '410415', '413810', '413999']
nosy_count = 4.0
nosy_names = ['vstinner', 'Mark.Shannon', 'corona10', 'silverjubleegaming']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46356'
versions = ['Python 3.11']

@vstinner
Copy link
Member Author

C extensions written for Python 3.10 and older which access directly to the PyFrameObject.f_back member build successfully on Python 3.11, but they can fail because f_back must not be read directly. f_back can be NULL even if the frame has an outer frame.

PyFrameObject*
PyFrame_GetBack(PyFrameObject *frame)
{
    assert(frame != NULL);
    PyFrameObject *back = frame->f_back;
    if (back == NULL && frame->f_frame->previous != NULL) {
        back = _PyFrame_GetFrameObject(frame->f_frame->previous);
    }
    Py_XINCREF(back);
    return back;
}

I suggest to remove or "hide" this member from the structure. For example, rename "f_back" to "_f_back" to advice developers to not access it directly.

See also bpo-46355: Document PyFrameObject and PyThreadState changes.

@vstinner vstinner added 3.11 only security fixes topic-C-API labels Jan 12, 2022
@vstinner
Copy link
Member Author

It's unclear to me if the ability to read directly the PyFrameObject.f_lineno member is a deliberate use case, or if we should enforce the usage of PyFrame_GetLineNumber().

@silverjubleegaming
Copy link
Mannequin

silverjubleegaming mannequin commented Jan 12, 2022

This must be normal

@markshannon
Copy link
Member

f_lineno is not part of the API. No field in PyFrameObject is.

I don't know how we can stop people from using them though.

If they don't know better than pulling data out of undocumented internal struct, then I'm not sure an underscore prefix is going to help.
It won't do any harm, though.

@vstinner
Copy link
Member Author

I don't know how we can stop people from using them though.

The first option is to promote helper functions to abstract access to the PyFrameObject structure.

The second option is to make the whole structure opaque and enforce the usage of helper functions. Before being able to do that, we need to prepare Cython and a few more popular C extensions for that. Right now, Cython still has a lot of code accessing directly PyFrameObject members.

Well, we can discuss that in bpo-40421 ;-)

If they don't know better than pulling data out of undocumented internal struct, then I'm not sure an underscore prefix is going to help. It won't do any harm, though.

I propose to rename f_back to trigger a build error for existing C extensions which access it directly. So developers have to decide how to handle the change: access the renamed member, or read the doc and use a better way, call PyFrame_GetBack() :-)

I dislike that this incompatible change is "silent" and that developers may only notice the change in production, when it's too late.

Or maybe most C extensions have a a good test suite and will notice the change before it's too late.

@vstinner
Copy link
Member Author

I created bpo-46836: "[C API] Move PyFrameObject to the internal C API".

@vstinner
Copy link
Member Author

This issue has been fixed by bpo-46836:

New changeset 18b5dd6 by Victor Stinner in branch 'main':
bpo-46836: Move PyFrameObject to pycore_frame.h (GH-31530)

@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
3.11 only security fixes topic-C-API
Projects
None yet
Development

No branches or pull requests

2 participants