Use ImagingCore.ptr instead of ImagingCore.id by homm · Pull Request #8341 · python-pillow/Pillow

homm · 2024-09-01T22:21:14Z

Partially suppress #8340

The non-primary extensions in Pillow don't have direct access to Python's _imaging types, such as Imaging_Type and ImagingObject. However, they often still need to interact with objects passed from Python code. Currently, the raw memory pointer to an Imaging object (ImageCore.id) is passed as an integer and then cast to a memory pointer:

static PyObject *
cms_transform_apply(CmsTransformObject *self, PyObject *args) {
    Py_ssize_t idIn, idOut;
    Imaging im, imOut;

    if (!PyArg_ParseTuple(args, "nn:apply", &idIn, &idOut)) {
        return NULL;
    }

    im = (Imaging)idIn;
    imOut = (Imaging)idOut;

    return Py_BuildValue("i", pyCMSdoTransform(im, imOut, self->transform));
}

It's not safe and can lead to segmentation faults. Python has Capsule objects, which provide a safer way to pass pointers to C code. These are exposed through ImageCore.ptr.

This PR deprecates the raw pointer properties ImageCore.id and ImageCore.unsafe_ptrs, and migrates non-primary extensions to interact with Capsule objects.

PytestUnraisableExceptionWarning: Exception ignored in: <function PhotoImage.__del__> AttributeError: 'PhotoImage' object has no attribute '_PhotoImage__photo'

homm · 2024-09-02T09:59:12Z

@python-pillow/pillow-team I need help here)

Could anyone debug pypy3.10 on windows? I can't get what's wrong, since as I can see, the capsule object is correctly restored from the pointer and the capsule object itself is alive.

https://github.com/python-pillow/Pillow/actions/runs/10664609198/job/29556218383?pr=8341#step:27:3342

homm · 2024-09-02T11:05:39Z

Get it! In pypy repr(capsule) holds the capsuled pointer itself, not the pointer to the PyObject. And even hex(id(capsule)) is not the pointer to PyObject. Lucky, we can reliably distinguish pypy capsule from cpython capsule by absence of trailing "<>".

https://github.com/pypy/pypy/blob/branches/py3.10/pypy/objspace/std/capsuleobject.py#L39

radarhere · 2024-09-02T21:17:59Z

So is the motivation here just to simplify the code?

Also, we don't necessarily need the deprecations - #4532 (comment)

Pillow makes a commitment to stable public interfaces, which are defined at the Python layer. The C interfaces are explicitly internal, and no effort is made to keep them stable even between minor releases, and no support or warning is given when they change. (IIRC, there are/were people using the internal interfaces, but they've always been explicitly on their own with that)

homm · 2024-09-03T07:48:16Z

So is the motivation here just to simplify the code?

Added description to PR.

Also, we don't necessarily need the deprecations

While ImageCore is internal Pillow object, it's still Python API. I believe ImageCore.id could be used by third-party extensions (Github search) and can't be removed in a one day.

Use getim()

homm · 2024-09-22T09:55:01Z

What's the advantage of using repr(capsule) in the first place? Why not just pass the capsule itself all the time?

I'm not aware of TK API, but it seems it converts any arguments to strings, at least PyImagingPhotoPut receives arguments as const char **argv. So looks like there is no way to pass the capsule object itself, instead repr(capsule) will be passed any way. I just made this conversion more explicit.

src/PIL/ImageTk.py

Co-authored-by: Andrew Murray <[email protected]>

radarhere · 2024-09-30T21:51:29Z

it seems it converts any arguments to strings, at least PyImagingPhotoPut receives arguments as const char **argv. So looks like there is no way to pass the capsule object itself, instead repr(capsule) will be passed any way. I just made this conversion more explicit.

In CPython, sure, but from your comments, not in PyPy.

It seems simpler to note that CPython converts the capsule to a string,
rather than converting to a string and noting that PyPy doesn't convert it?

Depends on whether your thinking was already aligned with how Tk works, I guess. I've created uploadcare#148, but feel free to say you prefer the current version.

homm · 2024-10-01T11:29:51Z

but from your comments, not in PyPy.

That isn't in my comments. In both implementations, the Capsule is converted to a string. In the case of CPython, it's a string like <capsule object "Pillow Imaging" at 0xffff>, while in PyPy, it's a string like capsule object "Pillow Imaging" at 0xeeee. Where 0xffff is PyCapsule object address, and 0xeeee is the pointer stored in the capsule itself.

radarhere · 2024-10-01T11:33:13Z

// Special case for PyPy, where the string representation of a Capsule
// refers directly to the pointer itself, not to the PyCapsule object.

Oh, I see, I very much misunderstood this comment.

radarhere · 2024-10-03T10:53:11Z

Get it! In pypy repr(capsule) holds the capsuled pointer itself, not the pointer to the PyObject.

Interestingly, I guess this is what is preventing you from checking the capsule name against the magic with PyCapsule_IsValid() in ImagingFind().

if (strncmp(name, expected, strlen(expected))) { is has already performed the essence of that check by that point though, right?

homm · 2024-10-03T11:07:24Z

This check is the best we can do here. It’s not as secure as PyCapsule_IsValid(), since it only verifies that the passed string is a correctly formatted string representation of the Capsule object. It’s still possible to craft a well-formatted string with the wrong pointer, which could result in a segmentation fault when calling a Tk operation. However, it’s still a significant improvement over the previous implementation, which simply obtained some number and assumed it was a pointer.

src/_imaging.c

src/libImaging/Imaging.h

Tests/test_image_getim.py

src/_imagingmorph.c

radarhere · 2024-10-07T06:32:00Z

For the sake of interest, unsafe_ptrs was originally added in #476 for PyAccess (which was removed in #8182). id has been there since PIL.

I'm not aware of TK API, but it seems it converts any arguments to strings, at least PyImagingPhotoPut receives arguments as const char **argv.

I looked, and this would be mirroring https://www.tcl.tk/man/tcl8.6/TclLib/CrtCommand.htm

src/_imaging.c

Co-authored-by: Andrew Murray <[email protected]>

docs/deprecations.rst

docs/releasenotes/11.0.0.rst

Co-authored-by: Hugo van Kemenade <[email protected]>

homm added 4 commits September 2, 2024 01:24

Use PyCapsule in _imagingmath

2303c06

Use PyCapsule in _imagingcms

a9798e7

Use PyCapsule in _imagingmorph

2fcab26

Use s# in PyArg_ParseTuple

f246be7

homm force-pushed the use-ptr branch 5 times, most recently from 1fddcce to 9bd6434 Compare September 2, 2024 01:00

homm added 2 commits September 2, 2024 05:14

Deprecate ImageCore.id and ImageCore.unsafe_ptrs

7435a06

Remove legacy 1-bit api, fix AttributeError

c69ad03

PytestUnraisableExceptionWarning: Exception ignored in: <function PhotoImage.__del__> AttributeError: 'PhotoImage' object has no attribute '_PhotoImage__photo'

homm force-pushed the use-ptr branch 5 times, most recently from 830a245 to 491e637 Compare September 2, 2024 09:33

homm force-pushed the use-ptr branch 3 times, most recently from 10db853 to bc83e33 Compare September 2, 2024 10:53

homm added 2 commits September 2, 2024 15:09

Use PyCapsule in _imagingtk

920c4ac

Use PyCapsule in _imagingft

147f75e

homm force-pushed the use-ptr branch from bc83e33 to 147f75e Compare September 2, 2024 11:09

rename PyCapsule -> Capsule

bf11639

homm force-pushed the use-ptr branch from 4eb58da to bf11639 Compare September 3, 2024 07:55

homm and others added 2 commits September 5, 2024 01:04

Move new_block to module

8833548

Merge branch 'main' into use-ptr

8dcf229

Use getim()

9f409e8

radarhere mentioned this pull request Sep 21, 2024

Use getim() uploadcare/pillow-simd#147

Merged

homm added 2 commits September 22, 2024 13:18

Merge pull request #147 from radarhere/use-ptr

87414b3

Use getim()

Fix hasattr for ImageTk.PhotoImage.__del__

11bcd5a

radarhere reviewed Sep 26, 2024

View reviewed changes

src/PIL/ImageTk.py Outdated Show resolved Hide resolved

Catch AttributeError for BitmapImage.__photo

b9d1768

Co-authored-by: Andrew Murray <[email protected]>

radarhere mentioned this pull request Sep 30, 2024

Do not use repr() in ImageTk uploadcare/pillow-simd#148

Closed

radarhere reviewed Oct 5, 2024

View reviewed changes

src/_imaging.c Outdated Show resolved Hide resolved

radarhere reviewed Oct 5, 2024

View reviewed changes

src/_imaging.c Outdated Show resolved Hide resolved

radarhere mentioned this pull request Oct 6, 2024

Random crashes of python in latest dev version of Pillow with pillow_heif #8439

Closed