[gnutls-dev] Re: living without global variables

[Simon Josefsson]

bryanh at giraffe-data.com (Bryan Henderson) writes:

>>The entire tree is constant as well.
>
> That's great.  I really couldn't tell just what information is in there.
> What is it, anyway?  Something you can explain quickly?

It is the ASN.1 schema for various PKCS and PKIX standards, in a
compiled form for libtasn1.  The variables are constant and read-only
throughout the lifetime of the GnuTLS library, except for when it is
deallocated in gnutls_global_deinit().

Perhaps GnuTLS should deallocate these variables in an `atexit' hook
instead.  Then it seem all of this problem would go away.  In GNU
libc, atexit hooks are even called if GnuTLS was dlopen+dlclose'd.
gnutls_global_deinit() would then only call gc_done().

> The information from Nikos that reference counting will be implemented
> solves another part of the problem (but I'm a little skeptical about that
> because the old code seemed to be pretty deliberate in not doing reference
> counting; it actually looks like there was reference counting before and
> someone undid it for some reason).

It simply looked half-finished to me.  With my latest fix, I think
this problem has been solved.  If you are concerned with the
theoretical memory leak, we could adopt the atexit solution.

> But there's still more work to do.  It isn't thread-safe.  Often, a
> program is modular in that two threads are executing separate code
> independently.  If both of those threads decide to use the facilities
> of the gnutls library, they might simultaneously call
> gnutls_global_init() and the reference count would get screwed up.

I'm not sure I see the problem here.  GnuTLS should in general be
thread safe.  Allocating and deallocating these two global read-only
variables may be the only thing causing problems.  Let's consider if
both threads call gnutls_global_init() at the same time.  As far as I
can see, the worst thing that can happen is that there is a race in:

  if (_gnutls_init++)
    goto out;

between checking the variable and incrementing it.  If there is a
thread context switch between checking the variable and increment it,
so that both threads read the value 0 from the variable, the worst
thing that seem to happen is that the global variables are allocated
twice.  That is a memory leak, but a pretty insignificant one, and one
that would happen rarely.  There is no serious problem, that cannot
easily be solved by the caller, as far as I can see.  Nothing else bad
will happen, as far as I can see.

> On the other hand, in what I propose each thread has its own memory
> and there can't possibly be a conflict.  (The synchronization point
> moves to malloc(), which is a reliable place for it).

Alternatively, each thread could do a mutex around the call to
gnutls_global_init:

 mutex_get()
 gnutls_global_init();
 mutex_release();

Although I wonder if the complexity is worth it.  The only danger in
not doing this is that if there is a thread race, you could leak some
memory.

>>Remember that API changes are bad.
>
> But API _additions_ are no big deal.  I'm talking about adding three
> new functions for people to use optionally in place of three existing
> ones, and people who use the existing ones will see no difference at
> all.

Good!

>>I think it would be useful to see exactly what changes you are
>>thinking of in more detail.
>
> I don't want to go so far as coding a patch if you don't buy into the
> basic idea, because it would be too much work.  But let me try a more
> detailed explanation.  This is nothing revolutionary -- it's how lots
> of libraries work in order to cater to threaded, modular, object
> oriented programs.

There are a lot of different solutions to this problem, so I wanted to
know exactly which one you thought of.

For example, many libraries has chosen to only support one thread
library, and then use mutexes from that thread library to solve
problems like this.  GnuTLS could have done that too, and wrapped
gnutls_global_init and gnutls_global_deinit around thread mutexes.
But I believe that is too inflexible for GnuTLS, consider this:

Application A (uses NPTL)
   Library B (uses pthreads)
     GnuTLS
   Library C (uses GNU Pth)
     GnuTLS

If GnuTLS used a pthread mutex internally to protect against this, it
would not work well.

I believe it is best to write thread safe code.  If that can't be done
(and I think we may change the GnuTLS code here), declaring that the
function is thread-unsafe and relying on the caller to protect the
call around a mutex is the second best bet.  Of course, if there is a
good argument for another approach, I could change my mind.

> A program calls gnutls_create_session_group() instead of
> gnutls_global_init().  This new function returns a "session group" handle.
> The program then calls gnutls_session_init_group() instead of
> gnutls_session_init().  This new function takes that session group handle
> as an argument.  When it's all done, the program calls
> gnutls_destroy_session_group(handle) instead of gnutls_global_deinit().
>
> The session handle is really a pointer to a descriptor (struct) that
> contains root pointers for the "global" trees.  So each session group
> has its own copy of the trees.
>
> The session group handle gets added to the session descriptor, and
> everything in gnutls that references the trees references them via the
> relevant session group descriptor instead of direct reference to
> global variables.  Some internal functions need to get an extra
> parameter so they know which tree to access.
>
> So that the existing API keeps working, there is a global session
> group descriptor that takes the place of the two global tree pointers.
> gnutls_global_init() does a gnutls_create_session_group() of that
> descriptor and gnutls_session_init() does a
> gnutls_session_init_group() with the global handle.

This could work, but I don't exactly see what the problem is that
would warrant all those changes.  The only gain I see is that you
protect against a potential memory leak in gnutls_global_init() that
will most likely never happen in real code.  For comparison, the
constant memory leak inside libgcrypt should be more worrying to
you...

>>I'm not sure exactly what real problem you are having.
>
> I thought the basic global variable issue was widely enough known that
> there wouldn't be a need to describe a particular scenario, but I'll
> tell you where I'm coming from.

Read-only constant global variables are not a serious problem as far
as I know.

I would have completely agreed with you if these variables were
modified by any GnuTLS functions.  gnutls_global_deinit() does that,
but it is a rather special case to me.  And the atexit approach seems
like an easier solution anyway.

> I don't have a program that crashed.  I distribute a library
> (XML-RPC for C/C++) that uses gnutls.  (Actually, it's worse.  It
> uses Curl, and Curl uses gnutls).  My users want to write object
> oriented programs with multiple threads.  I know that because Curl
> has a global init function, and one reason it has that is because
> gnutls does, I can't give my users that because my code has to
> initialize gnutls and I have no way to keep my gnutls initialization
> from conflicting with gnutls initializations that the user does for
> other purposes (it's the simultaneous reference count update problem
> I described above).

Understood.

> The best I can do is expose the deep underlying use of gnutls (no easy
> thing, since gnutls is only one of Curl's SSH options and Curl is only
> one of Xmlrpc-c's HTTP options) and tell the user he has to supply an
> already initialized gnutls environment to me.

That seem to be a layer violation.  I think it could be avoided.

> But with the session group concept, it's a snap.  I just give each
> Xmlrpc-c client object its own Curl client object and each Curl client
> object its own gnutls session group, and I can't possible interfere
> with the user's other uses of gnutls.  The only thing I'm sharing with
> them is the code of gnutls.

My claim is that with the current code, it should be a snap anyway.

Thanks for discussing this,
Simon