Fix segfault when reading mp4 files #1078
Conversation
Fix a problem where any mp4/mpeg-4 file will cause cmus to segfault when read. This was because of a recent change in `libmp4v2` that changed the MP4Read function from an overload with a filename/atomic callback to a C++ default argument with nullptr, which then caused a segfault when called without the second argument.
Clang possibly requires the MP4Read function to be cast for it to compile, so the MP4Read call has been changed to reflect that. The #ifdef has also been reintroduced. Just in case.
Add a return statement to the casted MP4Read call. Also remove the ifdef since it is not needed.
Add an __APPLE__ ifdef to the MP4Read call so that MacOS's libmp4v2 can be called properly [apparently it doesn't have a second argument]. Not sure how effective this is.
Forgot to close the ifdef. Whoops.
Quickly clean up some of the comments of this fix.
Turns out that libmp4v2 was actually updated on MacOS to add that new second argument. This means that the build should pass. Oh well.
This reverts commit 0860527. The CI didnt work, they still havent fixed this issue.
Use the MP4V2 version instead of the OS variant to determine whether to use the new API Co-authored-by: Niko E. <nefthy@users.noreply.github.com>
Now that this MP4Read fix is practically finalized, make sure we properly document why we make this call.
|
Okay, that's an absolute joke. At this point, we have three options:
I'm in favor of 3. However, cmus is already in dire need of a maintainer, so adding an mp4v2 fork on top of that would make matters worse. If no action is taken I'll likely just drop this PR entirely since there really seems to be no good solution to this that doesn't involve a fork of some kind [a task that I'm really not ready for given that I'm juggling two other projects right now]. |
That is not a good Idea. Linux systems could also use the abandoned upstream.
What package managers do is not in under our control.
I don't think pulling mp4v2 into cmus is a good idea. We should test at configure time which mp4v2 we have and do a #typedef. gcc -x c -Wall -Wextra - <<EOF
#include <mp4v2/mp4v2.h>
int main() {
MP4Read("/dev/null", NULL);
return 1;
}
EOF
echo $?gives a result of 1 on my system, which is not using the techsmith fork. |
|
Hm. Good idea. I'll try to take a crack at it later. |
|
Okay, I got blind-sighted by other projects and forgot about this PR, but now I'm actually going to try to add a configuration for this, like you suggested. Give me some time and I'll try to make this PR actually become workable. |
Add a block to the configuration script that seems to adequately detect for the techsmith fork. Basically, if we can't detect the header but can still confirm that the library exists through try_link, we just assume it's the techsmith fork since by default the busted header won't compile.
|
I give up. I can't get the configure script working at all. For some reason the mere act of adding my fix causes the CI to repeatedly fail, continues trying to import |
Cmus will encounter a segmentation fault reading mp4/m4a/m4b if the shared
libmp4v2library is at v4.1.4+. This is because that release changed theMP4Readfunction from an overloaded method to a single method with a default argument ofnullptr, causing cmus to segfault when it only specified one argument.This PR fixes that by changing the function call to specify
NULLas the second argument, which prevents cmus from segfaulting.Fixes #1076.
Related: mp4v2/#47
Feel free to raise any concerns with this fix.