egl-wayland icon indicating copy to clipboard operation
egl-wayland copied to clipboard

Improve wlEglMemoryIsReadable behavior

Open mackyle opened this issue 3 years ago • 3 comments

The implementation of the recently added wlEglMemoryIsReadable function will not always check to see whether or not the entire range is readable thanks to some optimizations by the write function.

These patches make its check much more robust (although still not perfect) and handle edge cases such as EINTR, EAGAIN and EINVAL that the original implementation does not while adding only a very tiny (constant) amount of extra work.

With these changes it should almost always provide the correct answer, even on the *BSDs and without an assertion failure either.

mackyle avatar Sep 20 '21 01:09 mackyle

In general, out of curiosity, have you actually encountered any problems that prompted this MR? Or was it just based on inspection of the code?

erik-kz avatar Sep 27 '21 17:09 erik-kz

Also, I was aware when making this change that trying to check a block of memory larger than the pipe buffer would fail. I didn't anticipate ever needing to do that, though, and so decided to avoid the extra complexity. The assert was meant to inform the programmer that such usage wasn't supported.

erik-kz avatar Sep 27 '21 17:09 erik-kz

I saw this code doing this "creative" thing that normally ought to result in a SIGSEGV but due to a non-POSIX-specified behavior extension of the write call allows memory addressing space to be inspected using only standard POSIX API calls, and, function calls that are guaranteed to be async-signal safe at that. Of course I snarfed that code out into a separate test program and played with it.

While I was only ever able to get an EFAULT out of linux I did convince a *BSD to give me an EINVAL with just the right size parameter (ridiculously large). However, without the added check of the final byte, the original code returns a "yes it's readable" result for pretty much any length as long as that first little bit is readable. It does not trigger the assertion in the original code -- I was only able to ever trigger the original assertion when I convinced a *BSD to give me an EINVAL. I still suspect that under very heavy resource contention that an EAGAIN might pop out on the initial write to the non-blocking pipe -- can't prove it's not possible without a kernel analysis on all the systems supported by the driver -- but I wasn't able to cause one.

Not checking for EINTR is just bad since the write call is documented as being able to return that. The documentation for the write function very clearly states:

If a write() is interrupted by a signal handler before any bytes are written, then the call fails with the error EINTR

Does wlEglMemoryIsReaable always run with all signals blocked?

Since PIPE_BUF is only the size of one memory page on Linux and the old code handled one page, I was thinking the only reason you got rid of the old code was because you needed to check more than one page for readability (or perhaps try to be more portable). You even say that in the commit comment about the new function: "which can check whether an arbitrarily sized block of memory is readable." As the original code is written, it definitely cannot do that. It can only check whether up to a PIPE_BUF size chunk of memory is readable.

Without the patches, it does not fail when trying to check a block of memory larger than PIPE_BUF, it succeeds thereby claiming it's all readable.

FreeBSD contains

sys/sys/syslimits.h: #define PIPE_BUF 512

The NVIDIA driver downloads site offers downloads for "FreeBSD x86" and "FreeBSD x64".

Is checking only 512 bytes sufficient? Because that's all the original code can guarantee it's checked, certainly not an "arbitrarily sized block of memory." And that only as long as a signal does not arrive at just the wrong time.

mackyle avatar Sep 27 '21 22:09 mackyle