Fix static lib registration by randombit · Pull Request #279 · randombit/botan

randombit · 2015-09-12T04:30:55Z

The ongoing support issues around the static library not behaving as expected are trouble; anyone who builds a static lib with any kind of custom build system will end up with a misbehaving library.

The issues with the registration system come from the static linker not considering the objects as needed at link time as there is no explicit reference in the code, so the objects are discarded and then lookups fail.

Instead group the registrations by type and in the same file include something which the linker will know is required, for example the destructor of the base type. Thus simply referencing a base type is enough to ensure the object registrations will occur regardless of static/shared.

With shared libs there is no real change here beyond a lot of extra #ifdefs since all objects are linked into the .so and are always visible and included. With static libs, more objects are pulled in than are strictly necessary, but much fewer than with the amalgamation.

Tests now pass with static non-amalgamation builds (verified by circleci change), so remove the restriction set in configure.py that requires amalgamation builds for static libs.

The support problems from having static libraries not work in the obvious way will be endless trouble. Instead have each set of registrations tag along in a source file for the basic type, at the cost of some extra ifdefs. On shared libs this is harmless - everything is going into the shared object anyway. With static libs, this means pulling in a single block cipher pulls in the text of all the them. But that's still strictly better than the amalgamation (which is really pulling in everything), and it works (unlike status quo).

With this change the tests pass when linked against a static library built in the normal (non-amalgamation) fashion. Remove the restriction in configure.py, and have circleci build the clang static build as a non-amalg.

cdesjardins · 2015-09-12T09:23:24Z

Nice work, I will certainly be giving this a try within the week! I still wonder if this issue could be resolved in a cleaner way with an API change. I haven't fully worked through it, so it could be a waste of time, but it might be a fun experiment one day.

randombit · 2015-09-12T14:04:21Z

Great. I'm going to leave this one open for review and testing for a while I think.

cdesjardins · 2015-09-13T09:47:06Z

I have started playing with this and it seems better, get_block_cipher() appears to work, and get_hash_function() appears to work, of course my testing is limited so far because there is still an issue with RNG:

#include "botan/botan.h"

int main(int argc, char** argv)
{
    std::unique_ptr<Botan::RandomNumberGenerator> rng(new Botan::Serialized_RNG());
    return 0;
}

When I run the above program I get:

terminate called after throwing an instance of 'Botan::Algorithm_Not_Found'
  what():  Could not find any algorithm named "HMAC(SHA-512)"
Aborted

I am using:

$ g++ --version
g++ (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

and configure with:

./configure.py --disable-shared --disable-modules=tls --prefix=$PWD/../install --libdir=$PWD/../install/lib/botan/Debug --build-mode=debug --disable-avx2 --maintainer-mode

I still haven't tried it on Windows yet, and I am totally open to suggestion if there is a preferred/better/working way to get a rng.

p.s. all tests in botan-test pass for me with the above configuration, and I do have the correct branch checked out, the git sha1 is: d83ef010522373a6f8ed3876c812b18b55513103

Previously we were hanging on the type destructors to pull in the relevant objects. However that fails in many simple cases where the object is never deleted. For every type involved in the algo registry add static create and providers functions to access the algo registry. Modify lookup.h to be inline and call those functions, and move a few to sub-headers (eg, get_pbkdf going to pbkdf.h). So accessing the registry involves going through the same file that handles the initialization, so there is no way to end up with missing objs.

randombit · 2015-09-17T22:50:29Z

There were some corner cases where the registration objects would not be pulled in. That should be fixed with my most recent push, and your test now works for me.

Only user-visible change is the removal of get_byte.h

cdesjardins · 2015-09-19T12:44:28Z

Alright, I had some more time to try this out again, and I tested with the e2e0f8f checkin, I haven't tried the "Initial header cleanups" patch because it wasn't there when I updated my branch this morning, and now it looks like it has CI failures anyways...

All that aside, I can confirm that I am able to connect to ssh servers with e2e0f8f, and the same configure as above using a bunch of different algorithms, and also using encrypted private keys. So for me this is a win, obviously it isn't a full test of everything in botan, but the subset that I use seems to totally work now with a static non-amalgam build.

Thanks!

webmaster128 · 2015-09-20T00:40:08Z

src/lib/pbkdf/pbkdf.h

Can we say that omitting braces is only allowed if the single statement is placed in the same line? Style Conventions

I'd be in favor of adopting this as a style rule.

In my opintion it is too easy to change a code like that

if (p) p->doOneThink();

to

if (p) p->doOneThink(); p->doAnotherThink();

without any notice by the compiler or anything. That risk can be reduced dramatically by requiring

if (p) { p->doOneThink(); }

or

if (p) p->doOneThink();

Especially the later versions fits great for null pointer checks like in that case.

webmaster128 · 2015-09-20T00:48:49Z

Great to see unique pointer factories, great to see pbkdf being remove from the base module and - of course – great to see static builds work!

I just started to play around with this by merging it into the cmake branch. With little adjustments, I was able to run the Botan tests in a static cmake build.

webmaster128 · 2015-09-20T00:56:25Z

src/lib/block/block_cipher.cpp

webmaster128 · 2015-09-20T01:39:12Z

I think we might need some clarity in factory function consistency. There are

HashFunction::create (nothrow, nullable, unique_ptr)
make_hash_function (throw, not,-nullable, unique_ptr)
get_hash_function (nothrow, nullable, raw pointer)

and

BlockCipher::create (nothrow, nullable, unique_ptr)
make_block_cipher (throw, not-null, unique_ptr)
get_block_cipher (nothrow, nullable, raw pointer)

but the make_* is missing for some cases, e.g. make_kdf, make_pbkdf don't exist:

BlockCipher::create (nothrow, nullalble, unique_ptr)
get_kdf (throw, nullable, raw pointer)

I personally prefer the object oriented style X::create. Checking for nullpointer or exception is no real difference for me. Is there a good reason for either choice? I also see no value in the raw pointers as those can be obtained easily from the unique_ptr.

So I'd opt for checking what we want and deprecate or delete the rest.

randombit · 2015-09-21T16:29:17Z

Consistency in the factory functions is non-existent, but the lookup.h signatures (with these inconsistent throw vs null returns) have been in place for some time which is why I kept them here. I'm much happier with the consistency of T::create and think the old functions should probably be deprecated and eventually removed. That is also the reason for not adding make_kdf, etc here.

Move the algorithm factory functions to T::create and move object registration to the source file for its base class. These resolve the issues which prevented successful use of a static library that was built with individual object files. Removes the restriction in configure.py which prevented building non-amalgamation static libs.

webmaster128 · 2015-09-21T20:52:14Z

Really good job, thanks @randombit!

…for-Sierra Editorial changes from Mountain Lion to Sierra.

coveralls · 2025-06-04T17:30:57Z

coverage: 90.561%. first build
when pulling 04319af on fix-static-lib-registration
into 8dc3b89 on master.

randombit added 4 commits September 10, 2015 01:45

Same treatment for cipher modes

8211fdc

Fix pbkdf, pk padding and ECDH registration for static linking.

72719f5

With this change the tests pass when linked against a static library built in the normal (non-amalgamation) fashion. Remove the restriction in configure.py, and have circleci build the clang static build as a non-amalg.

Fix for minimized builds

d83ef01

Internal header cleanups

e1ae1d6

Only user-visible change is the removal of get_byte.h

Fix includes for openssl

e94e615

webmaster128 reviewed Sep 20, 2015
View reviewed changes

src/lib/block/block_cipher.cpp Outdated

Copy link
Copy Markdown

Contributor

webmaster128 Sep 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent?

Address some review comments by Simon. GH #279

408ea0a

Remove use of lookup.h in favor of new T::create API.

04319af

randombit merged commit 7ebe751 into master Sep 21, 2015

randombit deleted the fix-static-lib-registration branch September 22, 2015 13:07

randombit mentioned this pull request Apr 6, 2016

Trouble with static initializers for Algo_Registry #465

Closed

0xa5a5 pushed a commit to 0xa5a5/botan that referenced this pull request Nov 8, 2019

Merge pull request randombit#279 from mikeneumann/update-macOS-notes-…

f604b62

…for-Sierra Editorial changes from Mountain Lion to Sierra.

randombit mentioned this pull request Jan 16, 2023

[TLS 1.3] Support signature_algorithm_cert Extension #3125

Merged

Conversation

randombit commented Sep 12, 2015

Uh oh!

cdesjardins commented Sep 12, 2015

Uh oh!

randombit commented Sep 12, 2015

Uh oh!

cdesjardins commented Sep 13, 2015

Uh oh!

randombit commented Sep 17, 2015

Uh oh!

cdesjardins commented Sep 19, 2015

Uh oh!

webmaster128 Sep 20, 2015

Choose a reason for hiding this comment

Uh oh!

randombit Sep 21, 2015

Choose a reason for hiding this comment

Uh oh!

webmaster128 Sep 21, 2015

Choose a reason for hiding this comment

Uh oh!

mouse07410 Sep 21, 2015

Choose a reason for hiding this comment

Uh oh!

webmaster128 commented Sep 20, 2015

Uh oh!

webmaster128 Sep 20, 2015

Choose a reason for hiding this comment

Uh oh!

webmaster128 commented Sep 20, 2015

Uh oh!

randombit commented Sep 21, 2015

Uh oh!

webmaster128 commented Sep 21, 2015

Uh oh!

coveralls commented Jun 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants