command: avoid crashing the player with mmap#17520
command: avoid crashing the player with mmap#17520N-R-K wants to merge 4 commits intompv-player:masterfrom
Conversation
N-R-K
force-pushed
the
rm-mmap
branch
2 times, most recently
from
91b1f58 to
95e1e62
Compare
N-R-K
force-pushed
the
rm-mmap
branch
2 times, most recently
from
754163a to
3866a07
Compare
|
What even was the point of accepting this weird 549a9ea sort of implies wanting to deprecate the whole |
It also allows the overlay source and mpv to be separate processes without writing files to disk.
|
N-R-K
force-pushed
the
rm-mmap
branch
4 times, most recently
from
b3939ce to
82b6e7c
Compare
player/command.c
Outdated
| bytesRead = fread(overlay.source->planes[0], 1, expectedBytes, fp); | ||
| } else for (int i = 0; i < h; i++) { | ||
| fseek(fp, src_off, SEEK_SET); | ||
| size_t r = fread(overlay.source->planes[0] + dst_off, 1, bytesPerLine, fp); |
There was a problem hiding this comment.
I think mmap was natural choice for this interface, as it not only simplifies the code, but reduces to syscalls bloat. Additionally in many cases only small ROI is copied from the source file.
Also theoretically you can mmap fd that is not seekable.
I understand mmap pitfals however, just pointing why it may have been desirable.
There was a problem hiding this comment.
Also theoretically you can mmap fd that is not seekable.
mmap is only defined to work with regular files and shared/typed memory. And in practice stuff like pipe doesn't work with mmap so I doubt requiring the fd to be seekable is introducing some new constrain.
There was a problem hiding this comment.
If you assume only seekable fd is supported, you can use pread and ReadFile with OVERLAPPED structure to read data which you need, instead of seeking the file itself, which may be unexpected for the user.
There was a problem hiding this comment.
This PR was initially doing pread except that windows doesn't have it.
ReadFile with OVERLAPPED structure
I did look into this, but it says:
For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.
This thing also seems to be tied to some async stuff? We obviously dont want the read to be async if the "fd" was async for some reason.
I am also a bit reluctant on doing windows stuff as I don't have an easy way to compile and test it locally. Making changes and pushing them to see if CI passes is a miserable state of affairs.
There was a problem hiding this comment.
For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.
Seeking is also required to be supported in current PR solution, so I see no difference.
This thing also seems to be tied to some async stuff?
Same as pread, it's an interface to allow multithreading, such that you don't modify global file state by seeking it, and allow multiple consumers to read different part of such file.
ReadFile itself is synchronous, there exist completelly async version with completion callback too (ReadFileEx).
I am also a bit reluctant on doing windows stuff as I don't have an easy way to compile and test it locally. Making changes and pushing them to see if CI passes is a miserable state of affairs.
That's life.
There was a problem hiding this comment.
@sfan5 had objection earlier to doing 1 syscall per line for the non-common case.
Is there any consensus on whether we should use pread and potentially take a performance hits on niche non-standard stride images or use buffered io and potentially risk breaking the api by seeking the fd.
I'm down (even if not enthusiastic) to try implement a pread windows shim but I want to know if that's the preferred method before I have to write windows code.
There was a problem hiding this comment.
I would still prefer avoiding pread. Seeking in the fd isn't that much of a problem.
We could even restore the original file position.
There was a problem hiding this comment.
We could even restore the original file position.
It'd be racy though. If we're going to seek let's just do it and document it instead of pretending as if we didn't seek.
There was a problem hiding this comment.
Sure. Just document that mpv may seek in the fd.
There was a problem hiding this comment.
Already documented:
Note that since mpv 0.42.0, if a file descriptor is specified, the file
offset of the descriptor will be modified.
_get_osfhandle returns INVALID_HANDLE_VALUE on failure, not 0.
according to the docs _get_osfhandle() sets errno to EBADF upon failure already: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=msvc-170
will be used in the next commit
mmap in inherently crashy due to the file changing underneath and requires esoteric SIGBUS handling [0] to use properly. to reproduce, simply use "overlay-add" from a lua script with invalid stride set and observe mpv crashing with SIGBUS. here we're just copying the file content (introduced in 549a9ea) and munmaping it immediately so there's no real need to be using mmap to begin with. avoid the issue entirely by just reading the file directly with fseek+fread with optimization for the common stride case. one notable difference is that since dup-ed fds share same offset, the source "fd" that's coming from client will now have it's offset modified. `pread` could avoid this issue but it's not supported on windows. [0]: https://www.sublimetext.com/blog/articles/use-mmap-with-care
4 participants
mmap in inherently crashy due to the file changing underneath and requires esoteric SIGBUS handling 1 to use properly.
to reproduce, simply use "overlay-add" from a lua script with invalid stride set and observe mpv crashing with SIGBUS.
here we're just copying the file content and munmaping it immediately so there's no real need to be using mmap to begin with. just read the file directly with pread (with optimization for the common stride case) and avoid the issue entirely.
Footnotes
https://www.sublimetext.com/blog/articles/use-mmap-with-care ↩