jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.06k stars 1.72k forks source link

Added script to generate all android versions and script to create AAR #1155

Closed ReenigneCA closed 2 years ago

ReenigneCA commented 2 years ago
  1. Added a very simple android-all.sh script that simply runs through all the android build scripts and runs them all.
  2. Added android-aar.sh script which builds an AAR with a prefab suitable for uploading to Maven (if you run the script on a stable codebase).
    • Existing users still have all the old build scripts;
    • New users can generate an AAR very easily that contains all useful combinations of build scenarios in a package that integrates with the major tools used for android development. See https://developer.android.com/studio/projects/android-library
    • builds static and dynamic versions of the library in both full and minimal mode and names them sodium sodium-static sodium-minimal and sodium-minimal-static within the AAR so it's easy to control static vs dynamic linkage for compilers or build systems that don't support specifying.
    • the x86 static library is a lie... it uses the dynamic library as the static library contains text relocations that won't allow it to link with android applications.

One item I wasn't totally sure of was the best way to set the libsodium version in the generated metadata as it seems to be hard coded in multiple places through the codebase? I leave that as a future cleanup item.

Edit: another useful link on AARs and prefabs

jedisct1 commented 2 years ago

Thank you for this!

Does android-all.sh have a use case besides building the AAR? If not, maybe it can be merged into android-aar?

Regarding the version number: it's defined in a single place, in configure.ac. There's a redefinition in msvc-scripts/process.bat but this is something I'd like to eventually get rid of.

ReenigneCA commented 2 years ago

Thinking it over it probably doesn't have a use case. It would if it pre-dated the AAR generation but if anyone uses all the android versions already they'd have already adapted to the existing scripts and anyone who doesn't yet use them would likely just want the AAR. It could easily just be a function inside the aar script. When I started writing this I thought there was a theoretical use case I probably could have just made the change :P now I've gotta prep for a meeting I'll try to make the change sometime tonight after work.

ReenigneCA commented 2 years ago

I couldn't help but notice that I was setting the stl to static or shared based on whether the library version was static or shared when it's actually about what C++ stl library the library uses (which being a c library is really none for sodium.) Testing that change lead me to notice that there is an issue with the AAR or potentially with AAR support in gradle.

With the x86_64 and arm7 emulators everything works as I expect. With the x86 emulator and with my Pixel 2 things seem to work, but it turns out it's only if you compile with the regular sodium version and then don't clean between future tests. What will happen is that if you test sodium::sodium first the libsodium.so for the full version will be added to the debug artifact and not removed when you change versions unless you manually do a clean in Android Studio. If you do run a clean then use sodium::sodium-minimal to link to the minimal version the executable will compile properly and the libsodium-minimal.so file will be added to the artifact but when the program is run it will insist on linking to libsodium.so instead of libsodium-minimal.so. If I just name all the shared artifacts libsodium.so then the build fails because they aren't named in accordance with their directory structure. The problem doesn't apply to static libraries except on x86 where static linking doesn't work and you really link to libsodium-static.so.

The example aar for openssl has two separate sub libraries and I follow that example in generating the aar though I suppose those sub libraries are compiled with the intention of being named the way they are instead of simply being renamed. The easiest answer is to generate multiple AARs with a single library named sodium within them. So instead of sodium::sodium-static it would be sodium-static::sodium in the CMAKE. Using '-' in the names might also be implicated as that is used to separate the library name from version number in the AAR naming scheme though I haven't tested this yet. I'm already late to cook supper now so I'll look closer at the issue and have this sorted along with moving the android-all functionality into the script sometime tomorrow or on the weekend.

ReenigneCA commented 2 years ago

I have basically no familiarity with gradle but I believe the issue I'm experiencing is related to two things. One is this:

https://github.com/google/prefab/issues/147

The other is that if you specify a libary name in a prefab the build system will only look for .so files with that name. I'm not entirely sure where this bug lies to be honest, maybe in prefab maybe in AGP maybe in CMAKE? Probably more of a feature request than a bug.

so when I name something libsodium-minimal.so it gets bundled with the apk but on some platforms the binary insists on using libsodium.so which isn't bundled. I can get around this by naming the library explicitly in the prefab metadata, but if I do so then the extension absolutely has to be .so or CMAKE won't find it... This wouldn't be a problem except for the whole x86 static linking lie since the x86 library won't work due to .text relocations. I can't specify a library name only for the x86 platform so if I specify it then only x86 works and if I don't then everything else works. Having multiple AARs solves the problem easily (I think,) I would really like the convenience of just having one AAR so I'm running down a few more posibilities.

I'm going to confirm that the lack of x86 compatibility still exists in master though if it doesn't anymore (I can't remember if I tested this,) that would mean that the script wouldn't build an AAR for the stable branch which I expect most people will want to use for crypto support.

The openssl package doesn't have this issue at all for some reason, or the package that google makes doesn't actually work... It isn't the exact scenario but it doesn't specify library names and it links against libssl or libcrypto and then bundles them. The problem may be as simple as being related to the names all starting with sodium. If I call the package saltedcrypto saltshaker or whatever and the modules sodium, sodium-minimal etc then the problem might go away. This is probably the best plan the name could be set to anything other than sodium if it works.

It seems to me that with the combination of problems I'm seeing if sodium is the static version then it will work because the library name will be the default so no need to specify. This would mean minimal static versions won't be put in but they'll be ltoed by default anyway so there minimal static versions are a niche use case I haven't ruled out that maybe someone might want. This would also mean that for static linking non x86 will have lto static libs and x86 will have the full non minimal version. My main issue is I don't like the naming convention of having sodium be static and sodium-shared being shared. If This approach catches on it also means that if the issues causing this are resolved (which seems to be an Android Gradle Plugin issue, though could be alleviated with a prefab feature to look for .a and .so names when a library name is specified I think,) changing back would break projects that use the odd convention. Though on the other other other hand it would just be changing one line in a build script and it wouldn't actually break break them just give them shared linkage when they expect static so increase their APK size silently break them.

Will re-re-attack when I get the chance. The renaming won't take long may be able to try it quickly over lunch or something.

ReenigneCA commented 2 years ago

After a lot of testing and trial and error I have determined what is going on and I think found the optimal solution.

TLDR is that I can now generate a file that works as long as I modify the android-build.sh file to add --disable-pie as per https://github.com/jedisct1/libsodium/issues/292. I am not entirely sure if this will have some sort of negative consequences and I am very confused by disabling pie eliminiating .text relocations but am willing to blindly accept that it works. I also need to add in some json files and change the naming convention for the sodium-minimal files.

Why? because There is no good way to have it swap in a .so for a .a when a platform can't support the .a file without generating a .so file with an alternate name that has a matching soname value inside the file itself. It seems significant changes to the existing build system (as far as I can find I don't have much autotools experience,) would be needed to do this with libsodium. On some platforms gradle will use the soname built into a .so file and the only way to get around it is to specify an explicit library name but in that case only .so files are supported. A change to the Android gradle plugin to use the real name (which given it knows the real name is the one it's putting in the apk it really shoud...) or one to the prefab tool (to allow specifying a library_name that relates to a .a file instead of a .so file) would allow this hackery without modifying the build chain. There is also a patchelf tool that can change the soname in the .so after it is compiled but there are issues on it's github about sometimes ruining .so files, or I could extract the .a file's .o files and generate a new .so file but I'm concerned about getting all the build flags right.

With all the potential solutions generating .a files that actually successfully link is the clear winner. The only question is why didn't the existing scripts already do this? Is there some reason someone might not want --disable-pie on?

ReenigneCA commented 2 years ago

I think I realise the deal with --disable-pie now. I suppose it must override fPIC and seems to be more lenient with text relocations. So I figured I was confident enough to just make the change and finish the script.

This latest commits:

I think this is ready for prime time. Might even want to consider asking the ndk ports people to use it and keep an AAR up to date in Maven for the project, though anyone who wants the prefab just needs to download, run autogen.sh -s then android-aar.sh. If a stable build is desired it should work with just copying the dist-build folder from master over the one in the stable version and then doing the same thing. To get it working with gradle you do all the things that you have to do for other AARs which are kind of annoying at least at this point (though once they are working they are the premium ndk library management solution). The best guide to using AARs I found is the one for oboe seems like the process has changed since prefabs were first introduced the c++ shared part might be optional I couldn't get things working without it earlier today but I've been varying a lot of things and that could at least in theory be based on something that is no longer relevant. I'm a bit wiped form tearing my hair out and ghidra-ing elf shared library files to figure out what was going on with this, I might get some time to work on an AAR specific readme file (maybe just text in the android-aar script that pops up at the end,) and confirming that this works with stable sometime this weekend.

ReenigneCA commented 2 years ago

My wife is currently sleeping in this morning so I figured I'd just finish off my personal wishlist for the script:

jedisct1 commented 2 years ago

Alright, so let's get this merged, then!

Thanks a ton for contributing this!

ReenigneCA commented 2 years ago

No problem. I use libsodium a lot and I'm glad I found a way to contribute something. thanks to you and the rest of the main contributors for making it happen.