[Replicant] [PATCH 0/1] EGL Loader patch to use both LLVMpipe and libagl at once

[Jeremy Rand]

>  From: "Jookia" <166291 at gmail.com>
> Sent: Monday, December 4, 2017 5:04 PM
> To: replicant at osuosl.org
> 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: jeremyrandmobile at airmail.cc
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 jeremy at veclabs.net is having technical issues at the
moment.





-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.osuosl.org/pipermail/replicant/attachments/20180116/b85a6614/attachment.asc>