Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wayland: Don't crash when the properties of already existing wl_output change #4038

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 1 comment · Fixed by #4305
Closed

wayland: Don't crash when the properties of already existing wl_output change #4038

SDLBugzilla opened this issue Feb 11, 2021 · 1 comment · Fixed by #4305

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 11, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 2.0
Reported for operating system, platform: Linux, All

Comments on the original bug report:

On 2021-02-01 06:10:33 +0000, Sebastian Krzyszkowiak wrote:

Created attachment 4747
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.

On 2021-02-01 16:58:00 +0000, Sam Lantinga wrote:

Patch added, thanks!
https://hg.libsdl.org/SDL/rev/7b0e5057af7d

On 2021-02-02 23:58:04 +0000, Christian Rauch wrote:

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.

On 2021-02-06 23:50:22 +0000, Christian Rauch wrote:

Created attachment 4769
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.

On 2021-02-09 00:06:19 +0000, Christian Rauch wrote:

memory leak fixed by https://hg.libsdl.org/SDL/rev/852a7bdbdf4b

On 2021-02-09 00:19:39 +0000, Sebastian Krzyszkowiak wrote:

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.

On 2021-02-09 22:28:49 +0000, Christian Rauch wrote:

(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?

@flibitijibibo
Copy link
Collaborator

flibitijibibo commented Apr 8, 2021

Resolved by e862856, the leak should probably be a separate issue if it still exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants