GNOME Bugzilla – Bug 769959
crash during WebP file loading
Last modified: 2016-08-17 14:51:03 UTC
Created attachment 333384 [details] repro WebP file for the crash The attached file make the webp-load plug crash. The problem comes from a double-free at plug-ins/file-webp/file-webp-load.c:210. Fix patch is on its way.
Created attachment 333385 [details] [review] fix for double-free on *indata fix patch attached.
Review of attachment 333385 [details] [review]: Bug and fix confirmed. Thanks for the patch! commit e4a2f220e0e7a868fa6f56b0cb82c0ee97ab1ba6 Author: skal <pascal.massimino@gmail.com> Date: Mon Aug 15 16:16:20 2016 -0700 Bug 769959 - crash during WebP file loading WebPDataClear() was called on already-free'd data.
Actually it would have been better to keep the WebPDataClear() and chuck the free instead. Odds are it will come back to haunt.
I actually wondered about this too and checked before validating and pushing the patch. But the libwebp API (https://developers.google.com/speed/webp/docs/container-api#webpdataclear) did not seem to indicate that WebPDataClear() had to be run in such a case. I can see only references of it to free data after WebPMuxGetFrame() and WebPMuxAssemble(). On the other hand, the indata had actually been allocated with g_file_get_contents() which requires a symmetrical g_free(). So that looked good to me. I did check the code of WebPDataClear() just now to make double sure and it indeed does just that. So I feel we may as well follow the documentation and keep the g_free(). Though in the end, they both do the same thing (as for freeing data). So it does not matter too much, I think. Now maybe I am missing something.
Correct, right now WebPDataClear() is currently calling free() (*not* g_free(). It might not be the same). But: - this might not be the case forever. One day, WebPDataClear() may be calling some other function symmetrical to some WebPData internal constructor or malloc. - actually, libwebp has a special memory-inspector that records the malloc/free when compiled with a special flag. If you call WebPDataClear() without the corresponding WebPSaffeAlloc() call from libwebp, the final balance will be off. Anyway, if the memory is allocated by gimp (through g_file_get_contents()), it *must* be de-allocated by a symmetrical gimp function, not one from libwebp.
Yep I agree (if that was not clear from previous message). So let's keep it as is. We can always come back on this later if needed, but for now, I think this is the right way. :-)