[Monotone-devel] Re: [Botan-devel] memory leak in Botan::Pipe?

[Jack Lloyd]

On Sun, Sep 10, 2006 at 02:16:17PM -0700, Nathaniel Smith wrote:
> 
> Whoops, right, Matt found that actually -- sorry, I forgot to send a
> followup.

No problem, glad you weren't blocking on my response.

> I'm still curious, if you have the time, about the bespoke heap
> management... do we know that it is helpful, and if so, when?

Ah, yes I forgot to reply to that portion initially. In some tests
(which were a significant while ago, but at least suggest why using
the pool makes sense in some cases), I found that code was using a
large portion of time in malloc(), which was reduced by using the
pool. That is probably because the various containers keep track of
the memory size anyway, and so it's a win to use the pool in the
common case, as it can rely on the user giving it the size back
correctly, which is basically 'free' (== already paid for). That
malloc() (particularly glibc's) scales better would not surprise me at
all. Summary: malloc() has a better asymptotic, the pool has a lower
constant.

I suspect the new memory allocator in 1.5 would handle this case
better. Deallocations are log n instead of linear, for one thing, and
the algorithms are less craptacular overall.  I'm currently running
tests to see how 1.5's allocators behave when something like you saw
happens, but they may take a while: large memory leaks + valgrind are
doing a nice job of killing my machine right now. To answer your
actual question, there is probably not any harm for Monotone to
replace the pooling allocators with a simple allocator that calls
malloc directly (make sure to memset the data before returning it (or
use calloc, I guess), as callers expect the allocator to do that).  In
1.4 that's add_allocator_type + set_default_allocator, in 1.5 it's
global_state().add_allocator(obj, true).

A patch that might actually be useful for you follows. Was playing
around last night with getting Monotone working under 1.5, where I
discovered some local changes to the private key loading function. You
can avoid the need for that with this patch (should work with 1.4.10
or 1.5.whatever, though I only have tried 1.5.11 - I ran the testsuite
after this change, BTW - all passed).

-Jack

# 
# old_revision [2be6b4d965b14f2a2221f729d2feb99647760137]
# 
# patch "keys.cc"
#  from [5b7c03118a6613b47cc5bd42cd8165f2db0ae91f]
#    to [1e1f21b4331dc99af8231cc1856ed33de67d2a3d]
# 
============================================================
--- keys.cc     5b7c03118a6613b47cc5bd42cd8165f2db0ae91f
+++ keys.cc     1e1f21b4331dc99af8231cc1856ed33de67d2a3d
@@ -19,6 +19,7 @@
 #include "botan/botan.h"
 #include "botan/rsa.h"
 #include "botan/keypair.h"
+#include "botan/pem.h"
 
 #include "constants.hh"
 #include "keys.hh"
@@ -288,8 +289,8 @@ migrate_private_key(app_state & app,
       try
         {
           Pipe p;
-          p.process_msg(decrypted_key);
-          pkcs8_key = shared_ptr<PKCS8_PrivateKey>(Botan::PKCS8::load_key(p, 
"", false));
+          p.process_msg(Botan::PEM_Code::encode(decrypted_key, "PRIVATE KEY"));
+          pkcs8_key = shared_ptr<PKCS8_PrivateKey>(Botan::PKCS8::load_key(p));
         }
       catch (...)
         {

As may or may not be obvious, I'm trying to lead you guys away from
using local patches, as then it will be easier for you to follow my
mainline and I won't get bug reports about stuff that I fixed 9 months
ago. :)