memory leaks

[ZIGLIO, Frediano, VF-IT]

Hi Marcus,
> 
> On Sun, 2006-01-08 at 14:33 +0100, Dirk Stoecker wrote:
> > as promised, I released the memory leak patch for gcrypt on 
> a webpage:
> > 
> > http://www.dstoecker.de/gcrypt.html
> > 
> > It includes the motivation for the patch, the way to 
> download and apply it 
> > as well as compatibility considerations (and a not for 
> usage in dynamic 
> > loading contexts).
> 
> Your website expresses a lot of frustration.  That is understandable,
> but I think we should try to approach this issue 
> constructively.  To do

I think that after some months a people can be a bit frustated :)

> so, we have to solve a complicated technical problem, given certain
> policy constraints.  If we can successfully identify these 
> constraints,
> and potential solutions, we have won a lot.  At least we will better
> understand why certain changes are rejected, and if we can develop a
> long term strategy.
> 
> It is very important that we untangle unrelated issues.  Your patch
> contains different types of changes intermixed.  One thing 
> you can do to
> push forward this issue is to split your patch into several different
> patches so we can address them individually.
> 

I agree, there are changes like configure or syntax (comma after last
enum entry) that are not related to finalization.

> One such issue is finalization.  It seems to me that your 
> code does not
> handle finalization correctly.  Consider the following case: 
> library bar
> and baz both use libgcrypt.  They both hide this fact from the
> application.  The application uses the library specific initializers
> (bar_init and baz_init) which each initialize libgcrypt.  
> Under certain
> conditions this is perfectly legit, in fact, it is the only way
> libgrcypt can be used correctly in such a situation.  Now, imagine baz
> comes with a finalizer, which calls the finalizer for gcrypt 
> you added.
> It seems to me from skimming over your patch that in this 
> case gcrypt is
> deinitialized.  This will potentially break bar's use of gcrypt (I did
> not check if it in fact does so).  It thus seems to me that your
> proposed solution in its current form is at best incomplete, 
> and I don't
> see a trivial fix.
> 

The usual way to fix this issue are mainly two
1- use counter
2- init library at so init

1 is usually preferred (it work even with static library). It consist in
using a counter in initialization. Counter is initially 0 and get
incremented on initialization and decremented on deinit. Initialization
occur only if you call init and counter get to 1

void init()
{
   if (++counter != 1)
     return;
  ... init ...
}

while deinitialization occur if counter get 0

void deinit()
{
  if (--counter != 0)
    return;
  ... deinit ...
}

(I not took into account thread problems)

2 require use of so. Just catch library load and initialize library and
deinit only at library unload. Very difficult to port on Unix systems.

> With regards to the current libgcrypt code, one overriding concern is
> ABI/API compatibility.  Irregardless of what we would _want_ 
> the code to
> look like, the current libgcrypt version is used in dozens of 
> important
> software packages, and making incompatible changes to the 
> ABI/API to the
> _current_ version would need to be carefully justified.  We feel that
> extension of the applicability of the library to new use cases (like
> dlopen'ed plugins) does not justify breaking existing use in dozens of
> software packages.  So any major changes, and my suspicion is that
> cleaning up the allocation issues involves major changes if 
> it is to be
> done right, would have to go into a major revision of the 
> whole package
> including the ABI/API.  This however raises a tail of other questions,
> like how do we carry over the confidence (security wise) that 
> we have in
> the current implementation to the hypothetical major revision, how can
> its development be supported, etc?

Well, current API is just broken for dlopen contexts... let me explain
why. Assume that library A call gcry_set_allocation_handler... now any
time you allocate memory inside libgcrypt you call alloc/free/realloc
provided by library A. Now what happens if library B calls
gcry_set_allocation_handler? Well, I think that all these globals should
be removed (same problem for threadsafe callbacks). 
There are some solution to this problem
- use only static library, any library that will use libgcrypt use a
different libgcrypt library (this break ABI where you provide dynamic
libraries)
- use a single solution (malloc/free/realloc from libc, pthread where
available) and do not use callbacks (ignoring settings), this keep ABI
but you can have some problems mixing calls to gcry_malloc/free/realloc
and "A malloc/free/realloc".
- use a session structure to store callbacks and change all API to
accept this session. This as you can imagine breaks API, however you can
avoid this using a global session and adding new APIs while old one call
new one adding global session, something like

return_type old_function(parameters)
{
   return new_function(global_session, parameters);
}

return_type new_function(session_type session, parameters)
{
   ... do what you want ...
}

you can "implement " old function using macro to keep API but no ABI

gcry_global_session_t gcry_global_session;
#define old_function(a,b,c) new_function(gcry_global_session,a,b,c)

This keep source compatibility.

you can keep ABI providing 2 library: the old one and a new one, on new
one (ie libgcrypting.so in Linux) we have only functions that use
session like

return_type new_function(session_type session, parameters)
{
   ...
}

while old library (that keep old names, ie libgcrypt.so on Linux)

return_type old_function(parameters)
{
   return new_function(global_session, parameters);
}

You can also use a mix of define and libraries so old programs compiled
with new libgcrypt will use new library just adding minimal changes
(define a program session somewhere and call functions in old way)

> 
> Equally important would be to find a convincing story how such a major
> revision could actually look like.  Moritz does have some ideas about
> how to restructure the code to make it more modular and less dependent
> on global statics.  However, some of the core issues are 
> unresolved.  We
> have some ideas, but I don't think we have actually found the best
> solution yet, and I am not sure what the best solution would 
> look like.
> 
> So here is how I suggest we proceed:  If you can identify separate
> issues in your patch, please report them as separate issues, 
> so they can
> be discussed separately.  In particular, I want to understand if your
> finalizer actually correctly solves the problem you identified (see
> above).  Furthermore, I would like to know if you see ways to improve
> the library in its current form without modifying the 
> ABI/API.  Last but
> not least, we can try to envision how an "ideal" libgcrypt would look
> like and see if we can find mechanisms that actually allow to 
> implement
> it.  This list of issues is losely sorted from easy to hard, and from
> high priority to low priority (from my perspective).
> 
> Thanks,
> Marcus
> 

bye
  Frediano Ziglio