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

Fix use-after-free: fix frame callback running after destroy_output #24

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

yorickvP
Copy link
Collaborator

I was seeing a segfault while testing, so ran it in valgrind.

==890== Memcheck, a memory error detector
==890== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==890== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==890== Command: ./slurp
==890==
==890== Invalid read of size 1
==890==    at 0x40294C: output_frame_handle_done (main.c:258)
==890==    by 0x813DA0D: ffi_call_unix64 (in /nix/store/qwmw5ddw8lk2pz4fy3irp87vj1cawvjh-libffi-3.2.1/lib/libffi.so.6.0.4)
==890==    by 0x813C9D2: ffi_call (in /nix/store/qwmw5ddw8lk2pz4fy3irp87vj1cawvjh-libffi-3.2.1/lib/libffi.so.6.0.4)
==890==    by 0x53826D3: wl_closure_invoke (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x537EEE8: dispatch_event.isra.6 (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x53803D3: wl_display_dispatch_queue_pending (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x53807FA: wl_display_roundtrip_queue (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x4022F6: main (main.c:514)
==890==  Address 0x90813ba is 74 bytes inside a block of size 224 free'd
==890==    at 0x4C2E638: free (in /nix/store/mbqm6g3j43qfcx6ql7hvb59mi5yvqxh9-valgrind-3.14.0/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==890==    by 0x402297: destroy_output (main.c:206)
==890==    by 0x402297: main (main.c:506)
==890==  Block was alloc'd at
==890==    at 0x4C2F6DA: calloc (in /nix/store/mbqm6g3j43qfcx6ql7hvb59mi5yvqxh9-valgrind-3.14.0/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==890==    by 0x4026AD: create_output (main.c:192)
==890==    by 0x4026AD: handle_global (main.c:336)
==890==    by 0x813DA0D: ffi_call_unix64 (in /nix/store/qwmw5ddw8lk2pz4fy3irp87vj1cawvjh-libffi-3.2.1/lib/libffi.so.6.0.4)
==890==    by 0x813C9D2: ffi_call (in /nix/store/qwmw5ddw8lk2pz4fy3irp87vj1cawvjh-libffi-3.2.1/lib/libffi.so.6.0.4)
==890==    by 0x53826D3: wl_closure_invoke (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x537EEE8: dispatch_event.isra.6 (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x53803D3: wl_display_dispatch_queue_pending (in /nix/store/iqcr4c8d2pmr19g4i8323jf8j48hrpvh-wayland-1.16.0/lib/libwayland-client.so.0.3.0)
==890==    by 0x40202A: main (main.c:430)

Fixed it by running wl_callback_destroy from destroy_output.

@yorickvP
Copy link
Collaborator Author

Seems to be #12.

Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Indeed this makes sense.

@emersion emersion merged commit f3bfd7b into emersion:master Feb 12, 2019
@emersion
Copy link
Owner

Thanks!

Seems to be #12.

The stack trace looks different.

@emersion
Copy link
Owner

The stack trace looks different.

Well, maybe it's fixed by this PR actually. You never know what happens when memory is corrupted.

@yorickvP
Copy link
Collaborator Author

@emersion it makes it all the way to send_frame, then causes this crash. That's what lead me to investigate. I have some core dumps with that stack trace.

@emersion
Copy link
Owner

This makes sense indeed. Let's assume it fixed it, we'll re-open if another codepath triggers it.

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 this pull request may close these issues.

2 participants