[Botan-devel] Segfault in current Debian unstable

Jack Lloyd lloyd at randombit.net
Fri Jan 22 09:17:43 EST 2010


On Fri, Jan 22, 2010 at 01:09:20PM +0100, Rickard Bellgrim wrote:
> Hi
> 
> I got a report from of my users of SoftHSM. He gets a segfault in current Debian unstable when he tries to generate a key. The backstrace makes it look like the problem is in Botan. He uses 1.8.8.
> 
> http://trac.opendnssec.org/ticket/75
> 
> Do you have any hints on how I should go forward?

I have replicated on my machine.

It's a bug in botan, but one that only comes up because of SoftHSMs
usage pattern, thus it never being caught before.

It looks like (at least in checks.cpp), the library is repeatedly
initialized and then shut down.

The functions in gmp_mem cache an allocator, it never gets
destroyed. (This is a bad design on my part)

So the sequence causing the problem is:

  initialize library
  first use of gmp, get an allocator and cache it
  later, shutdown the library

  initialize the library again
  gmp memory hooks already set; with a now deallocated Allocator
  try to do something, access a dangling pointer and boom

The zlib and bzip2 filters also hook into other libraries allocation
functions, but those hooks are per-stream; once deallocated, a new
allocator is always grabbed, so they handle this library
reinitialization scenario OK. gmp's, on the other hand, are static and
per-process.

One solution that first came to mind is for GMP_Engine's constructor
to set the functions, and then for the destructor to deallocate the
allocator and reset the memory functions. This would be an improvment
over status quo, but, it would mean that creating the engine would not
stack: you would die horrifically if you created another GMP_Engine
with another still living because the second constructor would
overwrite the pointer. Of course nobody actually does this, whereas
here we're breaking real code. Could also reference count and just
deallocate in destructor when there are no more users (using an
atomic<int> in C++0x or a Mutex + int in C++98).

I've attached a patch for 1.8.8 - can you test?

Ironically, just yesterday I told someone I was not planning on doing
any more 1.8 releases unless a 'real showstopper bug' showed up. I
should know when I'm tempting fate.

Here is the valgrind output that gave me the hint:

==28677== 1 errors in context 4 of 365:
==28677== Invalid read of size 8
==28677==    at 0x5225589: Botan::(anonymous namespace)::gmp_malloc(unsigned long) (gmp_mem.cpp:31)
==28677==    by 0x67B9D97: __gmpz_init (in /usr/lib64/libgmp.so.3.5.0)
==28677==    by 0x5227214: Botan::GMP_MPZ::GMP_MPZ(Botan::BigInt const&) (gmp_wrap.cpp:27)
==28677==    by 0x5226C7C: Botan::(anonymous namespace)::GMP_Modular_Exponentiator::GMP_Modular_Exponentiator(Botan::BigInt const&) (gmp_powm.cpp:27)
==28677==    by 0x5226E7C: Botan::GMP_Engine::mod_exp(Botan::BigInt const&, Botan::Power_Mod::Usage_Hints) const (gmp_powm.cpp:50)
==28677==    by 0x5281A14: Botan::Engine_Core::mod_exp(Botan::BigInt const&, Botan::Power_Mod::Usage_Hints) (pk_engine.cpp:164)
==28677==    by 0x52C01C0: Botan::Power_Mod::set_modulus(Botan::BigInt const&, Botan::Power_Mod::Usage_Hints) const (pow_mod.cpp:58)
==28677==    by 0x52BFF83: Botan::Power_Mod::Power_Mod(Botan::BigInt const&, Botan::Power_Mod::Usage_Hints) (pow_mod.cpp:19)
==28677==    by 0x52C09E9: Botan::Fixed_Exponent_Power_Mod::Fixed_Exponent_Power_Mod(Botan::BigInt const&, Botan::BigInt const&, Botan::Power_Mod::Usage_Hints) (pow_mod.cpp:142)
==28677==    by 0x52BFBE3: Botan::MillerRabin_Test::MillerRabin_Test(Botan::BigInt const&) (numthry.cpp:340)
==28677==    by 0x52BEA0D: Botan::passes_mr_tests(Botan::RandomNumberGenerator&, Botan::BigInt const&, unsigned int) (numthry.cpp:272)
==28677==    by 0x52BCC7A: Botan::random_prime(Botan::RandomNumberGenerator&, unsigned int, Botan::BigInt const&, unsigned int, unsigned int) (make_prm.cpp:75)
==28677==  Address 0x6a3c138 is 0 bytes inside a block of size 8 free'd
==28677==    at 0x4C2349D: operator delete(void*) (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==28677==    by 0x516D09A: Botan::Malloc_Allocator::~Malloc_Allocator() (defalloc.h:19)
==28677==    by 0x527C444: Botan::Library_State::~Library_State() (libstate.cpp:346)
==28677==    by 0x527AD02: Botan::set_global_state(Botan::Library_State*) (libstate.cpp:89)
==28677==    by 0x527AC29: Botan::LibraryInitializer::deinitialize() (init.cpp:72)
==28677==    by 0x4E32AC0: C_Finalize (main.cpp:230)
==28677==    by 0x4064AB: inittoken (checks.c:185)
==28677==    by 0x40657E: main (checks.c:74)

-------------- next part --------------
#
# old_revision [7fac653e942e8af70d705de6647d934761884b4a]
#
# patch "src/engine/gnump/eng_gmp.h"
#  from [ea4f6fec7a484240ceaef4978d19cb34032f3deb]
#    to [af0120eef40798f812091047dcbe18eb649e3cc5]
# 
# patch "src/engine/gnump/gmp_mem.cpp"
#  from [3c12c09a9ce0217087ebcd7694e16f49c367048f]
#    to [4b8f8568586a2de4087cfb0a0274a1be490dbdb0]
#
============================================================
--- src/engine/gnump/eng_gmp.h	ea4f6fec7a484240ceaef4978d19cb34032f3deb
+++ src/engine/gnump/eng_gmp.h	af0120eef40798f812091047dcbe18eb649e3cc5
@@ -48,8 +48,7 @@ class BOTAN_DLL GMP_Engine : public Engi
                                      Power_Mod::Usage_Hints) const;
 
       GMP_Engine();
-   private:
-      static void set_memory_hooks();
+      ~GMP_Engine();
    };
 
 }
============================================================
--- src/engine/gnump/gmp_mem.cpp	3c12c09a9ce0217087ebcd7694e16f49c367048f
+++ src/engine/gnump/gmp_mem.cpp	4b8f8568586a2de4087cfb0a0274a1be490dbdb0
@@ -48,23 +48,19 @@ void gmp_free(void* ptr, size_t n)
 }
 
 /*
-* Set the GNU MP memory functions
+* GMP_Engine Constructor
 */
-void GMP_Engine::set_memory_hooks()
+GMP_Engine::GMP_Engine()
    {
-   if(gmp_alloc == 0)
-      {
-      gmp_alloc = Allocator::get(true);
-      mp_set_memory_functions(gmp_malloc, gmp_realloc, gmp_free);
-      }
+   gmp_alloc = Allocator::get(true);
+   mp_set_memory_functions(gmp_malloc, gmp_realloc, gmp_free);
    }
 
-/*
-* GMP_Engine Constructor
-*/
-GMP_Engine::GMP_Engine()
+GMP_Engine::~GMP_Engine()
    {
-   set_memory_hooks();
+   mp_set_memory_functions(NULL, NULL, NULL);
+   delete gmp_alloc;
+   gmp_alloc = 0;
    }
 
 }


More information about the botan-devel mailing list