Closed Bug 1634209 Opened 5 years ago Closed 4 years ago

[RNP] librnp.so contains symbols newer than supported libstdc++

Categories

(Thunderbird :: Build Config, defect, P1)

Tracking

(thunderbird78 fixed)

RESOLVED FIXED
Thunderbird 79.0
Tracking Status
thunderbird78 --- fixed

People

(Reporter: rjl, Assigned: rjl)

References

Details

Attachments

(3 files, 4 obsolete files)

Bug 1633778 revealed that the initial Nightly builds of librnp.so on Linux contains symbols from a version of libstdc++ more recent than intended. For compatibility across as many Linux distributions as possible, the goal is to keep the minimum required version at 4.8.1, as that's what Botan requires.

Anything newer than GLIBCXX_3.4.19 is too new.

_ZNSt6chrono3_V212system_clock3nowEv (GLIBCXX_3.4.19)
_ZTINSt6thread6_StateE (GLIBCXX_3.4.22)
_ZNSt3_V216generic_categoryEv (GLIBCXX_3.4.21)
_ZNSt28__atomic_futex_unsigned_base19_M_futex_notify_allEPj (GLIBCXX_3.4.21)
_ZNSt28__atomic_futex_unsigned_base19_M_futex_wait_untilEPjjbNSt6chrono8durationIlSt5ratioILl1ELl1EEEENS2_IlS3_ILl1ELl1000000000EEEE (GLIBCXX_3.4.21)

I thought I had this, but my test build did not work.

Severity: -- → S2
Priority: -- → P1
Attached patch Drop MZLA_RNP_CC configuration (obsolete) (deleted) — Splinter Review
This was initially done for compatibility purposes, to support older Linux versions, but has proven ineffective and unnecessary.
Assignee: nobody → rob
Status: NEW → ASSIGNED
Attached patch Build Botan without thread support (obsolete) (deleted) — Splinter Review
I found that building Botan without thread support will result in a library that meets the minimum support criteria we are looking for. I did some light testing to see if there was an impact on performance by compiling Botan both with and without thread support and running "botan speed". There doesn't seem to be a significant difference. Anything else I should look at?
Attachment #9146021 - Flags: feedback?(kaie)
Attached image botan_chart.png (deleted) —

We need to check if we require support for multithreaded operation.

Inside the RNP codebase, the terms "thread" or "mutex" aren't used. Apparently RNP doesn't use threads by itself.

JavaScript allows worker threads. Currently, we don't use that with OpenPGP. However, in theory, it might be useful to perform costly operations, e.g. key generation, in a worker thread. But before we could do that, we'd have to check with the RNP library developers if that's a safe to do.

If we consider to use RNP with multiple threads, then the RNP implementation would have to rely on the Botan library to protect itself against races, too. With threads disabled, you don't get such protection.

So today, it might be ok to disable threading in Botan, but this requirement might change in the future. I will double check with the RNP developers and will give an update.

We got a response, RNP doesn't do any locking and doesn't have multithreading support currently.

However, RNP has the concept of a global library context, and when using separate contexts on separate threads, and if the application (Thunderbird) uses appropriate synchronization to avoid using both contexts in parallel, then it might be safe (from the RNP library perspective).

This still doesn't answer the question from Botan's perspective. I don't know yet if the Botan maintains any global state that it requires for its operation, which it would have to synchronize across threads.

The fact that Botan's library provides a configuration option for using threads, it might require the protection mechanism this config option provides (e.g. locks) for a safe operation.

So, I'm still a little nervous about your request. It might be fine, if you only require this as a workaround temporarily. But as soon as we'll start to use a worker thread for key generation in Thunderbird, we'll most likely need Botan to be built with support for threads switched on.

Rob, IIUC:

  • the usual approach to achieve certain minimum runtime requirements, is to use an old build environment, that matches the requirements
  • you are unable to use an old build environment, and therefore you're using tricks/hacks to minimize the requirements
  • although your tricks worked for some symbols earlier, they are no longer working, so you are searching for alternative solutions
  • the alternative solution you're currently suggesting is to disable thread safety in the Botan library.

If my understanding is correct, it might be preferable to avoid using a hacked approach, and avoid having to apply limitations to the runtime safety of the libraries we're using, and accept that we have to build the libraries in a separate environment.

Comment on attachment 9146021 [details] [diff] [review] Build Botan without thread support I'd prefer to avoid this approach. It's not just about performance, but also about consistency protections in a multithreaded environment, which Thunderbird is. We might easily get into a situation where a parallel execution is triggered, and we might memory corruption.
Attachment #9146021 - Flags: feedback?(kaie)

I've come up with some other ideas.

A) Build RNP and deps with the system GCC/libstdc++. This is GCC 4.9 with the recent updates to the base build image.
B) Build our own compiler. Lots of work, and probably not practical given time constraints.
C) Include libstdc++.so in the build. This is my least favorite given its size.
D) Statically link libstdc++ when building librnp.so. Surprisingly, after stripping its only an additional 1.4MB and seems to work.

Another option that did not work out
E) Convince Clang to use a different GCC/libstdc++. I tried, could not get this working. It may or may not work at all, and is still rather hackish.

I'm leaning towards option A. Option D seems fine. License though?

(In reply to Rob Lemley [:rjl] from comment #10)

A) Build RNP and deps with the system GCC/libstdc++. This is GCC 4.9 with the recent updates to the base build image.

And dynamically link with libstdc++ and don't bundle it?

Yes, this seems like the obvious choice. Was there a reason why you initially didn't want to use it? Did you assume you couldn't rely on anything outside the Mozilla build environment to be available?

B) Build our own compiler. Lots of work, and probably not practical given time constraints.
C) Include libstdc++.so in the build. This is my least favorite given its size.

Hmm, on my debian system that file is just 1.5 MB, but also not sure about the licensing terms.

D) Statically link libstdc++ when building librnp.so. Surprisingly, after stripping its only an additional 1.4MB and seems to work.

There is a library exception for the binary code, GCC RUNTIME LIBRARY EXCEPTION. We're probably already making use of it, if code is compiled with GCC, IIUC. However we'd have to be very careful to follow its rules, for example it mentions "Eligible Compilation Processes". Would we have to include the source code of libstdc++? Apparently not, but if we want to go there, we should probably involve a licensing expert to be certain. Better avoid if we can.

(In reply to Kai Engert (:KaiE:) from comment #11)

(In reply to Rob Lemley [:rjl] from comment #10)

A) Build RNP and deps with the system GCC/libstdc++. This is GCC 4.9 with the recent updates to the base build image.

And dynamically link with libstdc++ and don't bundle it?

Yes, this seems like the obvious choice. Was there a reason why you initially didn't want to use it? Did you assume you couldn't rely on anything outside the Mozilla build environment to be available?

Before this debian 8 update to the base build image, the version of gcc and libstdc++ was too old for Botan. Now that things have been updated a bit, this option becomes available.

B) Build our own compiler. Lots of work, and probably not practical given time constraints.
C) Include libstdc++.so in the build. This is my least favorite given its size.

Hmm, on my debian system that file is just 1.5 MB, but also not sure about the licensing terms.

Ahh yes, helps if I look at the final stripped copy doesn't it.

D) Statically link libstdc++ when building librnp.so. Surprisingly, after stripping its only an additional 1.4MB and seems to work.

There is a library exception for the binary code, GCC RUNTIME LIBRARY EXCEPTION. We're probably already making use of it, if code is compiled with GCC, IIUC. However we'd have to be very careful to follow its rules, for example it mentions "Eligible Compilation Processes". Would we have to include the source code of libstdc++? Apparently not, but if we want to go there, we should probably involve a licensing expert to be certain. Better avoid if we can.

Agreed. I saw those old bugs from pre-Firefox days. I'd prefer to not relitigate that whole thing.

Option A it is then unless I find some random broken thing forcing us to go with B.

Attempt to circumvent the whole libstdc++ symbols problem by replacing it altogether when building on Taskcluster. Since libc++ and its companion libc++abi are not as ubiquitous as libstdc++, they are statically linked. The libc++ license permits this quite explicitly.

Why did you move away from Option A ?

I had never heard of libc++ before. Are we sure it's sufficiently stable?

Library implementations can have subtle differences.
(e.g. https://stackoverflow.com/questions/40758070/difference-of-behavior-betwen-libstdc-and-libc-operator-on-bitset )

I don't know if Botan is tested with the libc++ library. It might be good to execute the "make check" target for Botan, to ensure our respective configurations are working as expected by the developers.

However, the make check target doesn't seem to be available in my local build, even though it's mentioned in the "On Unix" section on https://github.com/randombit/botan/blob/master/doc/building.rst

While reading that page, I found another very important detail. I had earlier recommended to use --minimized-build

I'm reading that it's important that we include the system_rng module, can you please ensure we build it? Thanks.

I still had an older Botan release locally. "make check" works with a current release.

Botan supports building with clang; clang is from the llvm project; the libc++ you're suggesting is also from the llvm project; I'm guessing that the clang c++ compiler uses libc++, too. If that's right, it seems ok to use libc++ in general.

Nevertheless, it would still be good to run "make check" as part of our build.

(In reply to Rob Lemley [:rjl] from comment #13)

Attempt to circumvent the whole libstdc++ symbols problem by replacing it
altogether
when building on Taskcluster.
Since libc++ and its companion libc++abi are not as ubiquitous as libstdc++,
they are
statically linked. The libc++ license permits this quite explicitly.

libc++ is from the LLVM project. It's gotten more of a following from the *BSD OS's due to its looser licensing than on Linux, but it's gaining more support recently on Linux. It's used now by Android, Fuschia, and Chromium[1] for the past couple of years. And of course OSX. It's been around since 2012 or thereabouts. GCC 11 is looking to support it as an alternative to libstdc++ as well [2].

The gcc approach was not working out as well as I'd hoped unfortunately, and I still had unexpected symbols showing up. I do have to set up a linux32 build of libc++ yet, but I will set that side for the moment and get 64 bit done first. This does not affect non-Taskcluster builds or Windows. OSX was using libc++ already of course.

The resulting librnp.so has symbols from glibc2.2.5 and libgcc3.0.0 IIRC. I should have this ready for formal review tomorrow as well as some quick test results from a handful of Linux distributions hopefully.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=724628
[2] https://www.phoronix.com/scan.php?page=news_item&px=GCC-11-libcpp-LLVM-Support

thanks, sounds good

(In reply to Kai Engert (:KaiE:) from comment #16)

Nevertheless, it would still be good to run "make check" as part of our build.
Agreed. My moonshot is to run the GTests in RNP as part of our existing GTest unittests. That may not be totally feasible due to the older version used by Mozilla. But, we need to run something from upstream for RNP and Botan would not hurt either to ensure that the shared library is viable and some Marionette (?) tests for the javascript. Oh and I have some code floating around to help with reviewing updates to the source code for all of these libraries too.

Attached patch bug1634209_static_libcxx_taskcluster.patch (obsolete) (deleted) — Splinter Review

Here is a working Linux64 patch that uses libc++ as described.

Only Linux64 will work, still dealing with Linux32.

Attachment #9146005 - Attachment is obsolete: true
Attachment #9146021 - Attachment is obsolete: true
Attachment #9150284 - Attachment is obsolete: true
Attachment #9154388 - Attachment is obsolete: true

There is no 32-bit libc++ built as part of the Clang toolchain, this builds
a standalone version using the same source tar file that the rest of the
Clang toolchain is built with.

Circumvents the problems with libstdc++ symbols problem by replacing it with
LLVM's libc++ only when building on Taskcluster. libc++abi is a required
companion library.
Since libc++ is not as ubiquitous as libstdc++ on Linux, it is statically linked.
The libc++ license permits this quite explicitly.
When librnp (and more precisely Botan) is compiled and linked this way, there are
no references to libstdc++ in the resulting librnp.so file.

Depends on D80223

Thanks for finding this solution. Please uplift to 78 beta when ready.

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/a09af0b4f7d2
Build lib32libc++ as a toolchain job. r=darktrojan
https://hg.mozilla.org/comm-central/rev/f55f8af7fe9e
Build librnp on Linux statically linking to LLVM's libc++. r=kaie

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

Comment on attachment 9157693 [details]
Bug 1634209 - Build librnp on Linux statically linking to LLVM's libc++. r?kaie

[Approval Request Comment]
Request is for these commits:
https://hg.mozilla.org/comm-central/rev/a09af0b4f7d2
https://hg.mozilla.org/comm-central/rev/f55f8af7fe9e

Attachment #9157693 - Flags: approval-comm-beta?
Target Milestone: --- → Thunderbird 79.0

Comment on attachment 9157693 [details]
Bug 1634209 - Build librnp on Linux statically linking to LLVM's libc++. r?kaie

Approved for beta

Attachment #9157693 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: