> From: "Jookia" <[email protected]> > Sent: Monday, December 4, 2017 5:04 PM > To: [email protected] > Subject: [Replicant] [PATCH 0/1] EGL Loader patch to use both LLVMpipe and > libagl at once > Hey Replicant! > > There's been an issue of having llvmpipe be too slow, so I cooked up a > patch > to allow people to use both LLVMpipe and libagl at the same time. > The patch follows and includes a large amount of documentation in the > code. > Apply it to frameworks/native.
I've briefly reviewed the code, and it generally looks sane. (Also cppcheck and clang-analyzer don't seem to see any issues, which is good.) C++ isn't my native language though, so I'd appreciate it if Wolfgang or someone else who's more comfortable in C++ than I am can take a quick glance at it to see if there are any issues I've missed. I haven't tried to build a full Replicant image with this patch. I've tested Jookia's prebuilt binary patch on my spare Galaxy S3 i9300, and it works as advertised. (Orfox actually feels a bit snappier than usual, which might be because the other apps aren't using llvmpipe, or it might be a case of the placebo effect.) There are two documentation issues that caused me minor trouble: 1. The documentation doesn't mention that setting an override requires root. At first glance it looked to me like only installing the patch required root, which caused me some confusion when the patch failed to find an override. 2. The example override specifies the wrong path for llvmpipe. It should be "/system/lib/egl/libGLES_mesa.so", not "/system/lib/libEGL/libGLES_mesa.so". (Happily, the logcat output made it pretty clear what the issue was here.) The collision issue due to the limited prefix length definitely triggers my OCD, and normally I would want a better solution. However, I checked to see how upstream AOSP handles this (since their "wrap" property has exactly the same issue to deal with), and AOSP appears to use a prefix match as well. (See https://github.com/apitrace/apitrace/issues/296 .) So, I think it's entirely legitimate to conclude that this is an upstream issue with AOSP, and that fixing this issue is not within Replicant's scope; thus a prefix match is acceptable here. One non-blocking suggestion is that it would be nice if we could use "setprop persist.egl /system/lib/egl/libGLES_mesa.so" (without an app identifier) to set the default renderer. This would be useful since the current method of switching the default renderer is rather hacky and doesn't survive a system update. Looking at how the patch is written, it looks like this would only be a line or two of extra work. A security note: I'm guessing that anyone who has privileges to set overrides can cause arbitrary shared libraries to be loaded into an application (by setting an override path that points to an attacker-controlled .so file). I don't think this is a security problem as the code is currently written, since setting overrides requires root, but this should be kept in mind if the code is ever extended to allow non-root users to set overrides. Anyway, ACK from my end -- but I would definitely like a C++-fluent Replicant developer to do a quick sanity check of the code prior to merging. Cheers, -- -Jeremy Rand Lead Application Engineer at Namecoin Mobile email: [email protected] Mobile OpenPGP: 2158 0643 C13B B40F B0FD 5854 B007 A32D AB44 3D9C Send non-security-critical things to my Mobile with OpenPGP. Please don't send me unencrypted messages. My business email [email protected] is having technical issues at the moment.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Replicant mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/replicant
