search

mono / libgdiplus

C-based implementation of the GDI+ API
http://www.mono-project.com/
MIT License
329 stars 171 forks source link

Fix double free SIGABRT with custom dash array handling #647

Closed cpw closed 4 years ago

cpw commented 4 years ago

Fix double free crash if you assign a custom dash array then switch back to a standard pattern.

filipnavara commented 4 years ago

This actually made me think. Should not the pen->dash_array be freed if it was previously set to custom value? What is the expected behavior when switching to standard pattern and then back to custom pattern using GdipSetPenDashStyle?

akoeplinger commented 4 years ago

Should not the pen->dash_array be freed if it was previously set to custom value?

We seem to be memcpy the custom dash array here so yeah I guess it makes sense that we'd free it: https://github.com/mono/libgdiplus/blob/2d10e5d0c44c5f7ce55656d4398bbe5825033fff/src/pen.c#L941

What is the expected behavior when switching to standard pattern and then back to custom pattern using GdipSetPenDashStyle?

I don't know and couldn't find any docs about that. If it was expected that switching back and forth retained the custom pattern then we shouldn't free.

filipnavara commented 4 years ago

We seem to be memcpy the custom dash array here so yeah I guess it makes sense that we'd free it:

That means it would currently leak the previous value in this method when switching from the custom pattern to a predefined one. I can submit PR to fix that.

If it was expected that switching back and forth retained the custom pattern then we shouldn't free.

The custom pattern gets overwritten by this method so the switch back would not work. I don't know if it should work though. Wine seems to retain the custom pattern but it's only consequence of different implementation.

The CI seemed to hard fail on this PR with a memory corruption error. I didn't look into it yet.