Streaming from URL ending in / causes memory corruption #11626
Comments
If mp_iconv_to_utf8 was given an empty string to convert in the buf parameter it would corrupt memory when writing a null into outbuf before returning it to the caller. This happened when streaming from a URL that ends in a slash. For such a URL the method mp_basename returns an empty string. The method append_dir_subtitles passes the result returned from mp_basename to mp_iconv_to_utf8 which then corrupts memory. This was detected using Guard Malloc. The fix changes mp_iconv_to_utf8 check up front if buf is empty and if it is return buf as the result in compliance with the documented behavior of the method when no conversion is needed. Fixes mpv-player#11626
|
I think this might also be related to #7834 it implicates the same line Btw compiling via |
|
I agree, this issue is a duplicate of issue #7834. I had not noticed that one. It was issue #11378 from @forthrin that caught my attention. Note that I did not compile with That came from the Today I had time to explore that. I ran these commands to configure mpv-build: printf "%s\n" --enable-debug > ffmpeg_options
printf "%s\n" -Dbuildtype=debug > mpv_options
printf "%s\n" -Db_sanitize=address,undefined >> mpv_optionsAnd then rebuilt The address sanitizer also identifies where the heap block was allocated. Full output below. Test run with address sanitizer: |
|
Wow! Well spotted. If this is indeed the true culprit, its ten year anniversary is nigh. It's stuff like this that explains the popularity of Rust. (Side note: I like the idea, but not the syntax.) I'll add the pull request pointer test before the culprit line with a printf to stderr and see if that concurs with the exception, though I assume the exception doesn't necessarily trigger every time because of the unpredictable order of memory allocations. Or are you able to trigger it every time? Update: The log statements are triggered for the video, but exceptions happen randomly, so I can't confirm correlation yet. I'll leave them in to check with other content and post updates. But in practice one of the devs will probably confirm if the PR resolves this. Better yet, someone should track down this |
|
The fix proposed in PR #11627 is under active review. I just streamed the example URL 3 times in a row using the released version of mpv. Played fine. As usual with heap corruption it depends upon what memory gets stepped on as to how and if the memory overwrite will manifest itself. The pattern of heap usage for applications that respond to asynchronous events and use multiple threads will vary from run to run. This makes memory corruption a critical problem that must be resolved. The good news is that the developer tools shown above are usually effective in identifying the offending code. In the case at hand the problem was detected in every test run I did. I took note of issue #11378 when it was first posted. I was busy at the time, so I only performed some limited testing which did not reproduce the problem. Since I had nothing to contribute I did not comment. A few days ago I was scanning open mpv issues looking to see if any new serious defects posted. I happened to check #11378 again and noticed the "keeps happening quite frequently" post. If something can be reproduced it can be tracked down. Thank you for being persistent and making sure this problem was noticed. |
If mp_iconv_to_utf8 was given an empty string to convert in the buf parameter it would corrupt memory when writing a null into outbuf before returning it to the caller. This happened when streaming from a URL that ends in a slash. For such a URL the method mp_basename returns an empty string. The method append_dir_subtitles passes the result returned from mp_basename to mp_iconv_to_utf8 which then corrupts memory. This was detected using Guard Malloc. The fix changes mp_iconv_to_utf8 check up front if buf is empty and if it is return buf as the result in compliance with the documented behavior of the method when no conversion is needed. Fixes #11626
If mp_iconv_to_utf8 was given an empty string to convert in the buf parameter it would corrupt memory when writing a null into outbuf before returning it to the caller. This happened when streaming from a URL that ends in a slash. For such a URL the method mp_basename returns an empty string. The method append_dir_subtitles passes the result returned from mp_basename to mp_iconv_to_utf8 which then corrupts memory. This was detected using Guard Malloc. The fix changes mp_iconv_to_utf8 check up front if buf is empty and if it is return buf as the result in compliance with the documented behavior of the method when no conversion is needed. Fixes mpv-player#11626
Important Information
Provide following Information:
Reproduction steps
In a debug version of mpv running under
lldbon the Mac and configured to uselibgmalloc.dylibstream this video:https://www.tv2.no/video/nyhetene/satt-ut-av-oppvaskmaskin-triks/1810348/
Before the mpv window appears execution will stop with a
EXC_BAD_ACCESS.I found that URL in the closed issue #10886.
Possibly this explains the heap corruption reported in issue #11378.
I built a debug version of mpv using mpv-build by doing this before building:
My debug session is shown below, but you may not need to look into it. This statement is corrupting memory:
mpv/misc/charset_conv.c
Line 232 in 4fd0a39
The method
mp_iconv_to_utf8has a bunch of input checking at the start of the method, but it is not checking to see if the string to convert passed inbufis empty. This code sequence inappend_dir_subtitles:mpv/player/external_files.c
Lines 188 to 189 in 4fd0a39
Is passing an empty string to
mp_iconv_to_utf8becausemp_basenamefinds the / at the end of the URL and returns an empty string. At least I think this is what is happening.I added this code to the start of
mp_iconv_to_utf8:With that change I was able to run
mpvunderlldbwithlibgmalloc.dyliband successfully stream the video.Once I get this issue posted and can refer to an issue number I will post a pull request with that change. I am a total newbie at debugging
mpvso a critical review of my analysis is needed.lldb session:
Expected behavior
mpvcan be run successfully using Guard Malloc.Actual behavior
EXC_BAD_ACCESS
Log file
mpv.log
Sample files
https://www.tv2.no/video/nyhetene/satt-ut-av-oppvaskmaskin-triks/1810348/
The text was updated successfully, but these errors were encountered: