Mailing List Archive

Hi Dirk,

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
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.

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.

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?

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



_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

Hello,

> > It includes the motivation for the patch, the way to download and apply it
> > as well as compatibility considerations (and a note for usage in dynamic
> > loading contexts).
>
> Your website expresses a lot of frustration. That is understandable,

Well, I tried to keep the level low. Usually it is very frustrating when
having a serious problem and being ignored completely. I am
nevertheless required to have this page released due to the LGPL license
of libgcrypt.

> 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.

That is a matter of fact, as it contains all the fixes necessary to
get the stuff running in our environment. I will not spend any amount of
time to make that perfectly seperated individual fixes when getting
ignored. I really have not enough time to be wasted.

Remove the finalization calls and the few related lines from the patch
and you get all the other memory leak based fixes. I will no do it, as I
need to assume it will get ignored again.

> 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.

An fix would be to have an initialization and finalization counter
and do delayed finalization. You have better insight into the topic to
find out, how this would be needed to be implemented, as reference
counting in the current state with automatic initializer calls is a bit
complicated.

Or simply forbid libraries to call finalize until a major redesign has
been done.

> 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.

Well, the idea of libgcrypt 2 to have an context based approach will
hopefully fix the related issues. Enforcing development in this area
would surely help a lot. But if there is no Alpha or Beta software nobody
will actually use it, you will not get comments and progress is slow.
Projects die due to slow developments. Making an alpha release with
interface description and design issues to solve would be a first step.

> 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).

I detailed above, why I will not do this. It works for our use as static
library. I followed the LGPL and released the code. I several times tried
to contact the libgcrypt maintaining and did not succed, so for me the
case is closed.

The patch is public domain, so you may use it (or parts of it) as you want
(including the necessarity to claim copyright by GNU). Or you do not.

If there are questions, I will answer them.

Ciao
--
http://www.dstoecker.de/ (PGP key available)

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

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

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu, 2006-01-12 at 09:36 +0100, Dirk Stoecker wrote:
> I am
> nevertheless required to have this page released due to the LGPL license
> of libgcrypt.

You are required to comply with the conditions of the LGPL. These
conditions say nothing about a web page.

> That is a matter of fact, as it contains all the fixes necessary to
> get the stuff running in our environment. I will not spend any amount of
> time to make that perfectly seperated individual fixes when getting
> ignored. I really have not enough time to be wasted.

If you want to have your patches applied to the main tree, then you are
asked to submit patches for separat issues individually, so we can
review them individually and accept/reject/ignore them on an individual
basis. This also helps you, because you will decrease your chances of
being ignored completely. You say you don't want to spend any amount of
time to make the patches perfect and separate when getting ignored.
However, why do you expect that we spend this time to make the patches
perfect and separate if we don't even know what they would do?

Developing free software cooperatively is not a game where one party
tries to get the other party to do the work for you. We did not ask you
to spend the work on these patches without prior discussion what would
be a good way to accomplish your goal. Consecutively, I think it is a
bit unfair to blame us for not accepting your patches and ignoring your
work.

> Remove the finalization calls and the few related lines from the patch
> and you get all the other memory leak based fixes. I will no do it, as I
> need to assume it will get ignored again.

A quick glance shows that this is not true. For example, there is an
unexplained modification to configure.ac. If you want a guideline for
what a good policy is to follow when submitting patches, have a look at
what is required from patch submitters to the glibc project.

> > One such issue is finalization. It seems to me that your code does not
> > handle finalization correctly.
>
> An fix would be to have an initialization and finalization counter
> and do delayed finalization.

I agree that this sounds like a promising strategy.

> You have better insight into the topic to
> find out, how this would be needed to be implemented, as reference
> counting in the current state with automatic initializer calls is a bit
> complicated.

I don't understand what's complicated about it. Can you elaborate?

> Well, the idea of libgcrypt 2 to have an context based approach will
> hopefully fix the related issues.

It might, but it's some time I talked to Moritz about it, and I don't
know if he addressed the shared global resource issues and how.

> Enforcing development in this area would surely help a lot.

Development is not enforced. It is done voluntarily, and/or funded.

Thanks,
Marcus



_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu, 2006-01-12 at 10:09 +0100, ZIGLIO, Frediano, VF-IT wrote:
> 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).

Yeah, 1 seems to be better.

> (I not took into account thread problems)

That's not so hard to do, as (1) the first init has to be done before
creating other threads using the library, and (2) all users must agree
on the thread library to use. So you can implement the necessary mutex.


> Well, current API is just broken for dlopen contexts...

Yes. Also, the current API is very stable, in the sense that we don't
want to modify it, for the reasons I gave. This means that we may not
be able to fix this issue for you in the current version.

> 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).

For the thread callbacks, this is not an issue, as you are required to
use the same threadsafe callbacks in all users. We thought about it,
and we could not think of a credible story where you would use more than
one thread library in a single program. Do you know one?

Allocation routines are a problem here, yes.

>
> 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)

You need to elaborate on that. I suppose you mean linking with some
sort of private scope, where every user gets their own copy of the
library. This is possible on GNU platforms, as far as I know, but not
at all portable. Am I wrong?

> - 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".

There are good reasons why some people want to avoid linking to pthread,
so we can't just require it. Also, we want to retain support for GNU
Pth. These are our design constraints. We considered creating library
variants "libgcrypt-pthread" etc, but does this really solve anything?
Would users start to introduce libopenldap-pthread, etc?

> - 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

Yes. We are now talking about a new project, though. Ie, a major
rewrite. Also, you can't just easily separate everything. You really
want to share your random pool as much as possible, because otherwise
you will be entropy drained very quickly.

Thanks,
Marcus



_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu, 12 Jan 2006, Marcus Brinkmann wrote:

> > I am
> > nevertheless required to have this page released due to the LGPL license
> > of libgcrypt.
>
> You are required to comply with the conditions of the LGPL. These
> conditions say nothing about a web page.

It is one of the cheapest forms of complying to LGPL/GPL.

> being ignored completely. You say you don't want to spend any amount of
> time to make the patches perfect and separate when getting ignored.
> However, why do you expect that we spend this time to make the patches
> perfect and separate if we don't even know what they would do?

Because I already had multiple time bad experience and don't believe
additional work will help.

I will no longer discuss this part about who is to blame for what as like
always this brings nothing for both sides. Any further discussions
restricted to the helpful parts of technical issues.

> > You have better insight into the topic to
> > find out, how this would be needed to be implemented, as reference
> > counting in the current state with automatic initializer calls is a bit
> > complicated.
>
> I don't understand what's complicated about it. Can you elaborate?

Reference counting would require exactly one INIT and one FINISH for each
party using the software. As there are "automagically" initialisations in
some calls, it may be complicated to track down different parties (e.g.
libraries, which do no explicit init call). I do not know the internals
well enough to decide how exact reference counting would be. It is always
more complicated to implement such stuff in existing code, where it has
not been designed from the very beginning.

This greatly depends on the real world usage of libgcrypt. Probably a
first step would be an assertion for these cases, so that the programmers
get an information and implement explicit init and shutdown calls. In
the time inbetween the finalize call would not release anything and a
configuration option in autoconf would toogle dummy/real-mode.

Maybe the best would be the introduction of "contexts" into the API/ABI as
additional option and having a global context for legacy applications as
Frediano explained in his mail. Using the deprecated attribute and proper
texts how to fix it in the header file will make updating of legacy
applications much easier. It would not break ABI and API.

In 2-3 years the deprecated stuff and the global context can be removed
(breaking the API) and the problem is solved. Thought before this the
new API must be designed or there will be incompatible changes for
libgcrypt 2 again.

Ciao
--
http://www.dstoecker.de/ (PGP key available)

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu, 12 Jan 2006, Marcus Brinkmann wrote:

> On Thu, 2006-01-12 at 10:09 +0100, ZIGLIO, Frediano, VF-IT wrote:
> > - 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
>
> Yes. We are now talking about a new project, though. Ie, a major
> rewrite. Also, you can't just easily separate everything. You really
> want to share your random pool as much as possible, because otherwise
> you will be entropy drained very quickly.

It's not so much a major rewrite. Only the few calls to global variables
must be redirect to context-based-calls.

The sharing can be done library internal:
- Only one global variable containing a list of contexts.
- If there is already a context, the shared data is taken from it.
- Only the last freed context releases the stuff.

Thought this again introduces strange memory issues and must be threading
save. I would prefer seperate contexts. There shouldn't be such a big
difference if f.e. two plugins in one program use the library or you have
two programs.

Ciao
--
http://www.dstoecker.de/ (PGP key available)

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu, 2006-01-12 at 15:05 +0100, Dirk Stoecker wrote:
> On Thu, 12 Jan 2006, Marcus Brinkmann wrote:
> > > You have better insight into the topic to
> > > find out, how this would be needed to be implemented, as reference
> > > counting in the current state with automatic initializer calls is a bit
> > > complicated.
> >
> > I don't understand what's complicated about it. Can you elaborate?
>
> Reference counting would require exactly one INIT and one FINISH for each
> party using the software. As there are "automagically" initialisations in
> some calls, it may be complicated to track down different parties (e.g.
> libraries, which do no explicit init call).

I understand now, thank you for the explanation.

Marcus



_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel