Memory leak in function `fname2char` due to new reference is not decreased (static analyzer report)

[Snape3058]

1. A new reference is returned and assigned to bytes.
   

   Pillow/src/Tk/tkImaging.c

   Line 371 in 68e39cb

   bytes = PyUnicode_EncodeFSDefault(fname);

2. Function returns without decreasing the refcnt of bytes.
   

   Pillow/src/Tk/tkImaging.c

   Line 375 in 68e39cb

   return PyBytes_AsString(bytes);

Internal Report ID: c6d448

[radarhere]

Would you mind providing some detail on how you ran the static analyzer? What tool did you use specifically?

[Snape3058]

It is an experimental analyzer of my unpublished research, which is developed on the top of the clang static analyzer.

[radarhere]

#6326 (comment)

I believe this change will cause undefined behaviour by reading deallocated memory.

From https://docs.python.org/3/c-api/bytes.html#c.PyBytes_AsString (emphasis mine):

char *PyBytes_AsString(PyObject *o)
Part of the Stable ABI.
Return a pointer to the contents of o. The pointer refers to the internal buffer of o, which consists of len(o) + 1 bytes. The last byte in the buffer is always null, regardless of whether there are any other null bytes. The data must not be modified in any way, unless the object was just created using PyBytes_FromStringAndSize(NULL, size). It must not be deallocated. If o is not a bytes object at all, PyBytes_AsString() returns NULL and raises TypeError.

My understanding of this is that the returned buffer will be freed with the decref call.

This would not be picked up by the Valgrind CI run as it is missing a display:

Tests/test_imagetk.py::test_kw SKIPPED (TCL Error: no display name and no $DISPLAY environment variable) [ 63%]
Tests/test_imagetk.py::test_photoimage SKIPPED (TCL Error: no display name and no $DISPLAY environment variable) [ 63%]
Tests/test_imagetk.py::test_photoimage_blank SKIPPED (TCL Error: no display name and no $DISPLAY environment variable) [ 63%]
Tests/test_imagetk.py::test_box_deprecation SKIPPED (TCL Error: no display name and no $DISPLAY environment variable) [ 63%]
Tests/test_imagetk.py::test_bitmapimage SKIPPED (TCL Error: no display name and no $DISPLAY environment variable) [ 63%]

[Snape3058]

@radarhere

I agree with this comment. It will lead to a UaF bug if the refcnt is decreased. However, it is also a fact that the PyObject is definitely leaked.

Maybe a better solution is required. Such as copying the content of the string into a temporary buffer, or just returning the PyObject directly, and using the getter function at the places where the content is used.