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

_imp.get_frozen_object possible incorrect use of PyBUF_READ #114685

Closed
sobolevn opened this issue Jan 28, 2024 · 6 comments
Closed

_imp.get_frozen_object possible incorrect use of PyBUF_READ #114685

sobolevn opened this issue Jan 28, 2024 · 6 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@sobolevn
Copy link
Member

sobolevn commented Jan 28, 2024

Bug report

It is documented that PyBUF_READ should be used with memoryview objects. But, it is used in PyObject_GetBuffer:

if (PyObject_GetBuffer(dataobj, &buf, PyBUF_READ) != 0) {

Other similar places that access .buf and .len just use PyBUF_SIMPLE, which I think we should use here as well.

I will send a PR.

Originally found by @serhiy-storchaka in #114669 (comment)

Linked PRs

@sobolevn sobolevn added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jan 28, 2024
@sobolevn sobolevn self-assigned this Jan 28, 2024
sobolevn added a commit to sobolevn/cpython that referenced this issue Jan 28, 2024
@serhiy-storchaka
Copy link
Member

Other question, why it was not caught in tests? Does PyObject_GetBuffer() ignore unknown flags or this code was never executed?

@sobolevn
Copy link
Member Author

PyObject_GetBuffer() expects a numeric flag, but it uses bitflag model with example logic:

    if (!REQ_FORMAT(flags)) {
        /* NULL indicates that the buffer's data type has been cast to 'B'.
           view->itemsize is the _previous_ itemsize. If shape is present,
           the equality product(shape) * itemsize = len still holds at this
           point. The equality calcsize(format) = itemsize does _not_ hold
           from here on! */
        view->format = NULL;
    }
    if (REQ_C_CONTIGUOUS(flags) && !MV_C_CONTIGUOUS(baseflags)) {
        PyErr_SetString(PyExc_BufferError,
            "memoryview: underlying buffer is not C-contiguous");
        return -1;
    }
    if (REQ_F_CONTIGUOUS(flags) && !MV_F_CONTIGUOUS(baseflags)) {
        PyErr_SetString(PyExc_BufferError,
            "memoryview: underlying buffer is not Fortran contiguous");
        return -1;
    }
    if (REQ_ANY_CONTIGUOUS(flags) && !MV_ANY_CONTIGUOUS(baseflags)) {
        PyErr_SetString(PyExc_BufferError,
            "memoryview: underlying buffer is not contiguous");
        return -1;
    }

PyBYF_READ is equal to 256, so it just contains all the features. PyBUF_SIMPLE is just 0, so no extras.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 29, 2024
…H-114686)

(cherry picked from commit 1ac1b2f)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 29, 2024
…H-114686)

(cherry picked from commit 1ac1b2f)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
serhiy-storchaka pushed a commit that referenced this issue Jan 29, 2024
) (GH-114701)

(cherry picked from commit 1ac1b2f)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
serhiy-storchaka pushed a commit that referenced this issue Jan 29, 2024
) (GH-114700)

(cherry picked from commit 1ac1b2f)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jan 29, 2024
PyObject_GetBuffer() now raises a SystemError if called with
PyBUF_READ or PyBUF_WRITE as flags. These flags should
only be used with the PyMemoryView_* C API.
serhiy-storchaka added a commit that referenced this issue Jan 31, 2024
PyObject_GetBuffer() now raises a SystemError if called with
PyBUF_READ or PyBUF_WRITE as flags. These flags should
only be used with the PyMemoryView_* C API.
@serhiy-storchaka
Copy link
Member

Unfortunately the value of PyBUF_READ intersects with PyBUF_INDIRECT, PyBUF_FULL and PyBUF_FULL_RO, so we cannot just ban this bit. But fortunately other values has other bits set, so this error is still distinguishable.

@sobolevn
Copy link
Member Author

What about PyBuffer_FillInfo(Py_buffer *view, PyObject *obj, void *buf, Py_ssize_t len, int readonly, int flags)? Do we need to check flags there as well?

@serhiy-storchaka
Copy link
Member

Maybe. We do not have examples of the misuse in this case, and it is much less used, but a similar error is possible here too.

@sobolevn
Copy link
Member Author

I will send an example PR, so we can decide 👍

sobolevn added a commit to sobolevn/cpython that referenced this issue Jan 31, 2024
sobolevn added a commit to sobolevn/cpython that referenced this issue Jan 31, 2024
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
PyObject_GetBuffer() now raises a SystemError if called with
PyBUF_READ or PyBUF_WRITE as flags. These flags should
only be used with the PyMemoryView_* C API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants