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 heap overflow with playlists #7946

Merged
merged 1 commit into from Jan 9, 2019
Merged

fix heap overflow with playlists #7946

merged 1 commit into from Jan 9, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 9, 2019

Possibly related to #7543?

This appears to be a long-standing memory corruption bug when deleting a playlist entry.

An example that reproduces the original issue under ASAN:

  • have a history playlist with exactly 100 entries (or whatever number the default size is set to?)
  • delete the first entry
  • crash:
==11463==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x621003e183c0 at pc 0x7f395257cdab bp 0x7ffef0b68440 sp 0x7ffef0b67be8
READ of size 4800 at 0x621003e183c0 thread T0
    #0 0x7f395257cdaa in __interceptor_memmove /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:720
    #1 0x55a49aa77c5c in playlist_delete_index /home/bp/RetroArch/playlist.c:122

Just in case I'm wrong, I wanted someone else to look at this and make sure it's right. My thinking on this is:

memmove() here is being used to take the memory for all the playlist entries (minus one), and move it all to the left, so that the second entry becomes the first, etc., and there is now an extra space at the end of the list. memmove takes a source, dest and size. In this case:

  • source is playlist->entries + idx + 1, the entry right after the selected one
  • dest is playlist->entries + idx, the location of the selected entry
  • size is (playlist->size - idx) * sizeof(struct playlist_entry), the number of bytes for every playlist item (including the one to delete)

In this case, it tries to move size entries (all 100) up one, starting at the top entry plus one. This means that it is wrongly reading one entry's worth of memory past the end of the array.

My change basically makes it read the number of entries minus one from the starting point and moves them up, since the idea is that we're deleting one and need to make room for it.

@ghost ghost requested a review from inactive123 January 9, 2019 04:27
@inactive123
Copy link
Contributor

Thanks for researching this. Hopefully this will fix the Switch playlist corruption issue as well.

@inactive123 inactive123 merged commit 6420ea8 into libretro:master Jan 9, 2019
@ghost ghost deleted the playlist branch January 9, 2019 15:06
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.

None yet

1 participant