| Summary: | wayland: Don't crash when the properties of already existing wl_output change | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Sebastian Krzyszkowiak <dos> |
| Component: | video | Assignee: | Christian Rauch <Rauch.Christian> |
| Status: | REOPENED --- | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | Rauch.Christian |
| Version: | HG 2.0 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Attachments: |
0001-wayland-Don-t-crash-when-the-properties-of-already-e.patch
fix memory leak in display callbacks |
||
Patch added, thanks! https://hg.libsdl.org/SDL/rev/7b0e5057af7d This patch added a memory leak: Direct leak of 104 byte(s) in 1 object(s) allocated from: #0 0x7f9e31ac59d1 in malloc (/usr/lib/x86_64-linux-gnu/liblsan.so.0+0xf9d1) #1 0x55fa7c4ee870 in SDL_malloc_REAL [...]/SDL/src/stdlib/SDL_malloc.c:5387 #2 0x55fa7c6b6118 in Wayland_add_display [...]/SDL/src/video/wayland/SDL_waylandvideo.c:324 #3 0x55fa7c6b64e8 in display_handle_global [...]/SDL/src/video/wayland/SDL_waylandvideo.c:399 #4 0x7f9e323b0ff4 (/usr/lib/x86_64-linux-gnu/libffi.so.7+0x6ff4) because the allocated "SDL_VideoDisplay *display" is not freed any more by "SDL_free(display)". Since "display" becomes the user data of the "output", it has to be freed when the output gets removed in "display_remove_global" or when SDL quits. The AddressSanitizer is now active in Debug builds. You should see new memory leaks caused by a patch by running some of the test exectuables. Created attachment 4769 [details]
fix memory leak in display callbacks
The patch "wayland: Don't crash when the properties of already existing wl_output change" opens a memory leak by not free-ing the 'display' any more.
The newly added AddressSanitizer reports this as:
Direct leak of 104 byte(s) in 1 object(s) allocated from:
#0 0x7fb49ba999d1 in malloc (/usr/lib/x86_64-linux-gnu/liblsan.so.0+0xf9d1)
#1 0x55ba26a8f870 in SDL_malloc_REAL [...]/SDL/src/stdlib/SDL_malloc.c:5387
#2 0x55ba26c570cc in Wayland_add_display [...]/SDL/src/video/wayland/SDL_waylandvideo.c:323
#3 0x55ba26c5749c in display_handle_global [...]/SDL/src/video/wayland/SDL_waylandvideo.c:398
#4 0x7fb49c384ff4 (/usr/lib/x86_64-linux-gnu/libffi.so.7+0x6ff4)
The attached patch fixes this issue.
memory leak fixed by https://hg.libsdl.org/SDL/rev/852a7bdbdf4b Your patch has just reintroduced the crashes. That "SDL_free(display);" was removed for a reason - although you were right that it should be freed at `display_remove_global` (I forgot it in my patch), your patch doesn't do it and causes use-after-free instead. (In reply to Sebastian Krzyszkowiak from comment #5) > Your patch has just reintroduced the crashes. That "SDL_free(display);" was > removed for a reason - although you were right that it should be freed at > `display_remove_global` (I forgot it in my patch), your patch doesn't do it > and causes use-after-free instead. Sorry, I see now that the callbacks get called again when e.g. the resolution changes. Could you post a patch that frees the display accordingly, so that we do not have any more memory leaks? |
Created attachment 4747 [details] 0001-wayland-Don-t-crash-when-the-properties-of-already-e.patch SDL's Wayland backend used to crash when the properties of some output changed - be it rotation, scale or anything else. This patch makes it possible for display_handle_done to be safely called multiple times for the same output.