Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused? networking libc symbols in Android libmegazord.so #3596

Closed
acatarineu opened this issue Sep 22, 2020 · 2 comments
Closed

Unused? networking libc symbols in Android libmegazord.so #3596

acatarineu opened this issue Sep 22, 2020 · 2 comments

Comments

@acatarineu
Copy link

acatarineu commented Sep 22, 2020

While working in the new Android version of Tor Browser based on Fenix we took a look at the built libmegazord.so and were expecting them not to contain any known libc networking symbol (e.g. from the list in https://searchfox.org/mozilla-central/rev/30e70f2fe80c97bfbfcd975e68538cefd7f58b2a/python/mozbuild/mozbuild/action/check_binary.py#217), since in Android all networking is done by the http client passed via RustHttpConfig.setClient at run time. But actually they did contain a few networking symbols, e.g. checking with readelf --syms :

94: 00000000     0 FUNC    GLOBAL DEFAULT  UND listen@LIBC (3)
96: 00000000     0 FUNC    GLOBAL DEFAULT  UND connect@LIBC (3
99: 00000000     0 FUNC    GLOBAL DEFAULT  UND accept@LIBC (3
...

I thought it would be nice to have some assurance that no rust code is calling networking functions directly and try to make sure that the built libraries do not include these (in principle unused) symbols. These seemed to be coming from NSS libnspr4.so, so I tried to build with cross-language LTO and indeed these were optimized out. This is a commit for our build system for that: https://gitlab.torproject.org/tpo/applications/tor-browser-build/-/merge_requests/71/diffs?commit_id=639b4c92a67db95f80b4b9fa5d2be38d54e1d588.

For this to work I had to:

I tested with rust 1.43 and application-services 61.0.13. Unfortunately, in newer versions using rust 1.45+ this is not working, I assume because it has moved to LLVM10 while the latest android toolchain is still in 9.

Is this (removing unused symbols from built libraries, in particular networking ones) something that you would be interested in achieving? In that case, I assume that a patch that builds with rust 1.44 (for LLVM 9) and lto=fat may be difficult to accept. Do you have any other ideas to achieve this symbol removal without cross-language LTO, or some way to make cross-language LTO work in a way that's acceptable for you?

┆Issue is synchronized with this Jira Task

@rfk
Copy link
Contributor

rfk commented Sep 23, 2020

I thought it would be nice to have some assurance that no rust code is calling networking functions directly.

This is a goal of ours as well, so thanks for digging in to the details here!

Is this (removing unused symbols from built libraries, in particular networking ones) something that you would be
interested in achieving?

In principle yes, I'm quite interested in landing something that can achieve this.

I believe we've had some issues with lto=fat in the past, ref #2535, #2531. I don't recall the details, and it may be that the only remaining concern was compile time. But still, we'd need to do some testing to evaluate.

Limiting to rust 1.44 seems undesirable though :-(

From the build-system patch you linked, do I understand correctly that you're building your own NSS that is linked by application-services, rather than using the NSS-building stuff we have in ./libs in this repo? One alternative I could suggest would be to hack up our NSS build so that it doesn't include these symbols in the first place, but maybe that won't help you given your build setup?

I also wonder whether you could solve this problem by forcing appservices to link to NSS dynamically. We go to some trouble to link statically in our default build (search for NSS_STATIC in this repo for the details) since we can't guarantee that consumers will ship their own copy of NSS, but for your purposes it might make sense to disable this and link dynamically against the nss that I assume you're building for Gecko.

@acatarineu
Copy link
Author

From the build-system patch you linked, do I understand correctly that you're building your own NSS that is linked by application-services, rather than using the NSS-building stuff we have in ./libs in this repo? One alternative I could suggest would be to hack up our NSS build so that it doesn't include these symbols in the first place, but maybe that won't help you given your build setup?

We don't use the the scripts from libs/ directly, but we follow them closely. So, we can hack up our own NSS build and then try to make a PR here and see if this is acceptable for mozilla.

I also wonder whether you could solve this problem by forcing appservices to link to NSS dynamically. We go to some trouble to link statically in our default build (search for NSS_STATIC in this repo for the details) since we can't guarantee that consumers will ship their own copy of NSS, but for your purposes it might make sense to disable this and link dynamically against the nss that I assume you're building for Gecko.

This is something we can try, thanks.

We'll spend some time on this and try to find something that is acceptable for Mozilla, and make a PR. It seems the cross-lto solution will not work long-term, since sooner or later there will always be discrepancies between the clang version used by rustc and the one in the android toolchain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants