Calling dash_info.fIntervals crashes if called after path_effect.asADash(dash_info)

[sherlockdoyle]

Describe the bug When a new skia.PathEffect.DashInfo() is created, all of its fields can be accessed. But once it is filled with a call of path_effect.asADash(dash_info), trying to access the fIntervals field throws a SIGSEGV error.

To Reproduce Steps to reproduce the behavior:

1. Run the following code

   import skia
   
   paint = skia.Paint(PathEffect=skia.DashPathEffect.Make([5, 10, 15, 20], 25))
   path_effect = paint.getPathEffect()
   dash_info = skia.PathEffect.DashInfo()
   print(dash_info.fIntervals)  # prints []
   print(path_effect.asADash(dash_info))  # prints DashType.kDash_DashType
   print(dash_info.fCount)  # prints 4
   print(dash_info.fPhase)  # prints 25.0
   # path_effect.asADash(dash_info)  # having a second asADash is useless
   print(dash_info.fIntervals)  # crashes with Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)

Expected behavior The last line should have printed [5.0, 10.0, 15.0, 20.0]

Desktop (please complete the following information):

• OS: Ubuntu 20.04.1 LTS
• Python: 3.8
• skia-python version: 87.3

Additional context Also tested on an Ubuntu 18.04.5 LTS system with Python 3.7.12 with the same results.

[sherlockdoyle]

Possible solution: Allow skia.PathEffect.DashInfo() to be initialized with a count, which will allocate the required memory for fIntervals.

[HinTak]

I don't think your suggestion is correct. According to https://github.com/kyamagu/skia-python/blob/dcba39ebd3708d1456ee2065cb71ebc7412361ca/src/skia/PathEffect.cpp#L253 , asADash is a simple upstream call. The observed problem I believe is because asADash modifies its argument's interior - this is likely documented behavior.

[HinTak]

According to https://github.com/kyamagu/skia-python/blob/dcba39ebd3708d1456ee2065cb71ebc7412361ca/src/skia/PathEffect.cpp#L148 which gives std::vector<SkScalar>(NULL, NULL + 0) and turns into a python None list. Then you try to do std::vector<SkScalar>(NULL, NULL + N) , which breaks? There must be a follow-up call somewhere doing the allocation in C usage?

I'd put a NULL check in this line, to stop the segfault. Not sure where the allocation should be yet.

[sherlockdoyle]

The correct way to call asADash is this

SkPathEffect::DashInfo info;
SkPathEffect::DashType type = pathEffect.asADash(&info);  // get fCount
if (info.fCount)
{
    info.fIntervals = new SkScalar[info.fCount];  // allocate memory
    self.asADash(&info);  // get fIntervals
}

See, how we need to make two calls and allocate memory manually. There is no way to allocate memory in C++ from Python, thus this method will always fail. Having a constructor/method which can allocate the memory would be ideal.

On the other hand, allocating memory manually is not very Pythonic. I've a personal project where I used to use skia-python. However, because of several issues and to have better control over the code, I now use my own skia bindings. I use a wrapper class around SkPathEffect::DashInfo and handle allocation and destruction.

https://github.com/sherlockdoyle/Animator/blob/ebd5e93f3f47cce5b841425fabed1c1afb4bb41b/skia/PathEffect.cpp#L144

[HinTak]

Okay, thanks for the code. I 'll stick that in for v119b4 in #205 (releasing in the next few days).

There is a 3-arg constructor of DashInfo, but I can't get it to take a python array and populate the fIntervals. If you have some ideas too on it, that would be nice.

[HinTak]

I think there should be a if info.fIntervals == nullptr there too, to stop memory leak, especially if we get a 3-arg DashInfo constructor to work. Anyway, I'll include this in #205 before merging/releasing.

[HinTak]

I don't seem to be able to allocate memory on the c++ side and have it visible back in python (it seems to be filled with garbage). Anyway, might need to continue for the next after v1119b4 is out. For v119b4 there is a check to avoid the segfault, so you can continue, but not yet getting the desired results.

I think we need to the python-side 3-arg constructor working to get the desireed outcome.

[sherlockdoyle]

asADash already checks fIntervals == nullptr internally.

In C++, fIntervals is supposed to be owned by the user. They are supposed to allocate and free the memory. In Python, at least, you would want Python to own this memory, and the lifetime of this memory should be tied to the dash-info object.

Even if you get the 3-arg constructor working, how would you free the allocated memory when the dash-info object is destroyed? At the very least, since I implemented in this way, you would need a wrapper class to handle the allocation and destruction.

[HinTak]

Your AsADash doesn't take a DashInfo argument... I was hoping that something along the line of fintervals = [5, 10, 15, 20] ; dashinfo = skia.Dashinfo(fintervals, 4, 25) can be made to work. Anyway, this is on-going, I am just going to put a change in which avoid segfault for v119b4, and come back to look at this later when I have more time.

[sherlockdoyle]

Yes, that will work too. See https://github.com/sherlockdoyle/Animator/blob/ebd5e93f3f47cce5b841425fabed1c1afb4bb41b/skia/PathEffect.cpp#L105

The data in Python lists are not laid out in a way that's compatible with a C++ array. As I said, you will need to allocate memory, copy the values and make sure to free the memory. Note that, in my py::init I directly take the list and don't convert to a vector. This is because, since I've to copy the data after all, it doesn't make sense to convert to a vector.

While this approach is feasible, I think that it is redundant. The fintervals you created will only be used to allocate the memory and copy over the data. When dashinfo changes, fIntervals will not change (unless you keep a reference to that and copy the data back).

I think, you can use a bytearray (or similar) to share memory, but you'd need to properly keep_alive them.