* implementor.texinfo, thread-safe.txt, threads.txt: Various updates relating to

thread support.

git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@16812 dc483132-0cff-0310-8789-dd5450dbe970

4 files changed, 184 insertions, 28 deletions

diff --git a/doc/ChangeLog b/doc/ChangeLog
index 9fd9fe0..985d2e0 100644
--- a/doc/ChangeLog
+++ b/doc/ChangeLog
@@ -1,3 +1,8 @@
+2004-10-06  Ken Raeburn  <raeburn@mit.edu>
+
+	* implementor.texinfo, thread-safe.txt, threads.txt: Various
+	updates relating to thread support.
+
 2004-09-07  Tom Yu  <tlyu@mit.edu>
 
 	* install.texinfo (Propagate the Database to Each Slave KDC):
diff --git a/doc/implementor.texinfo b/doc/implementor.texinfo
index a96e071..d9e00a6 100644
--- a/doc/implementor.texinfo
+++ b/doc/implementor.texinfo
@@ -474,18 +474,21 @@ another thread at the same time.
 
 A credentials cache, key table, or replay cache object, once the C
 object is created, may be used in multiple threads simultaneously;
-internal locking is done by the implementations of those objects.
-(Iterators?  Probably okay now, but needs review.)  However, this
-doesn't mean that we've fixed any problems there may be regarding
-simultaneous access to on-disk files from multiple processes, and in
-fact if a process opens a disk file multiple times, the same problems
-may come up.
+internal locking is done by the implementations of those objects.  (We
+assume that object destructors are invoked only when all other threads
+are finished with the object.)  @i{(Iterators?  Probably okay now, but
+needs review.)}  However, this doesn't mean that we've fixed any
+problems there may be regarding simultaneous access to on-disk files
+from multiple processes, and in fact if a process opens a disk file
+multiple times, the same problems may come up.
 
 Any file locking issues may become worse, actually.  UNIX file locking
 with @code{flock} is done on a per-process basis, and closing a file
 descriptor that was opened on a file releases any locks the process
 may have on that file, even if they were obtained using other,
-still-open file descriptors.
+still-open file descriptors.  UNIX file locks are used for credentials
+caches and keytab files; the replay cache implementation is already
+known to be unsafe in not using file locking.
 
 We MAY implement --- but haven't yet --- a ``fix'' whereby open files
 are tracked by name (and per object type), and a new attempt to open
@@ -496,6 +499,46 @@ but it may be ``good enough.''
 
 GSSAPI ....
 
+Strictly speaking, the GSSAPI specification says nothing about thread
+safety, so for best portability, a GSSAPI application probably should
+not assume that a GSSAPI implementation is thread-safe in any way.  On
+the other hand, the GSSAPI specification doesn't explicitly say that
+it's safe to use in a program that uses the Berkeley sockets API,
+either; at some point, you have to start making some assumptions.
+
+A GSSAPI security context, like a @code{krb5_context}, may be used
+only in one thread at a time.  The GSSAPI specification gives precise
+definitions of C data structures for buffers, object identifiers, OID
+sets, and channel bindings, that do not allow for the addition of a
+mutex.  Thus, these objects must not be modified by one thread while
+in use by another.  (All of the GSSAPI functions that modify these
+types of objects should be obvious; they're listed as ``modify''
+parameters in the specification.  In fact, aside from the case of
+@code{gss_add_oid_set_member}, they're generally output arguments,
+with the previous value ignored.)
+
+The function @code{gss_add_cred} can modify the
+@code{input_cred_handle} object, if a null @code{output_cred_handle}
+argument is supplied.  Thus, all @code{gss_cred_id_t} objects must
+have mutexes, and all accesses (except in the functions creating or
+destroying them) must acquire the mutex first.
+
+Note that the use of @code{const} in the GSSAPI C bindings is not a
+useful guide to when an object might or might not be modified.  In
+most cases, @code{const} is applied to handle arguments, which are
+defined as arithmetic or pointer types.  It applies to the argument
+itself, not the data pointed to @i{if} the type is a pointer; this
+would mean that the GSSAPI function in question cannot modify the
+value of its handle parameter, and puts no constraints on
+modifications to the object indicated by the handle.  And according to
+the C type compatibility rules, the function definition can omit those
+@code{const} qualifiers anyways.@footnote{If you're thinking that this
+means the use of @code{const} in the GSSAPI C bindings is confusing
+and/or useless, you're right.}
+
+
+
+
 @node Thread System Requirements, Internal Thread API, Kerberos API Thread Safety, Thread Safety
 @section Thread System Requirements
 
diff --git a/doc/thread-safe.txt b/doc/thread-safe.txt
index fbff9de..9e30706 100644
--- a/doc/thread-safe.txt
+++ b/doc/thread-safe.txt
@@ -18,7 +18,8 @@ in general, been written.  There are hooks, but often they ignore some
 of the library's local storage, mutexes, etc.
 
 If shared library finalization code doesn't get run at all at dlclose
-time, then you'll get memory leaks.  Deal with it.
+time, or if we can't use it because the execution order is wrong, then
+you'll get memory leaks.  Deal with it.
 
 Several debugging variables that are not part of our official API are
 not protected by mutexes.  In general, the only way to set them is by
@@ -26,6 +27,13 @@ changing the sources and recompiling, which obviously has no run-time
 thread safety issues, or by stopping the process under a debugger,
 which we blithely assert is "safe enough".
 
+Debug code that we don't normally enable may be less thread safe than
+might be desired.  For example, multiple printf calls may be made,
+with the assumption that the output will not be intermixed with output
+from some other thread.  Offhand, I'm not aware of any cases where
+debugging code is "really" unsafe, as in likely to crash the program
+or produce insecure results.
+
 Various libraries may call assert() and abort().  This should only be
 for "can't happen" cases, and indicate programming errors.  In some
 cases, the compiler may be able to infer that the "can't happen" cases
@@ -37,9 +45,23 @@ when dealing with other errors are handled by calling abort, for lack
 of anything better.  We should probably clean those up someday.
 
 Various libraries call getenv().  This is perfectly safe, as long as
-nothing is calling setenv or putenv or what have you.  Of course, that
-severely curtails the ability to control our libraries through that
-"interface".
+nothing is calling setenv or putenv or what have you, while multiple
+threads are executing.  Of course, that severely curtails the ability
+to control our libraries through that "interface".
+
+Various libraries call the ctype functions/macros (isupper, etc).  It
+is assumed that the program does not call setlocale, or does so only
+while the program is still single-threaded or while calls into the
+Kerberos libraries are not in progress.
+
+The Windows thread safety support is unfinished.
+
+I'm assuming that structure fields that are never written to (e.g.,
+after a structure has been initialized and *then* made possibly
+visible to multiple threads) are safe to read from one thread while
+another field is being updated by another thread.  If that's not the
+case, some more work is needed (and I'd like details on why it's not
+safe).
 
 ----------------
 
@@ -56,8 +78,6 @@ until after the acknowledgement.  This could be fixed with
 multiple-reader-one-writer type locks, but that's a bit more
 complicated.
 
-The Windows thread safety support is unfinished.
-
 The string returned by error_message may be per-thread storage.  It
 can be passed off between threads, but it shouldn't be in use by any
 thread by the time the originating thread calls error_message again.
@@ -65,6 +85,16 @@ thread by the time the originating thread calls error_message again.
 Error tables must no longer be in use (including pointers returned by
 error_message) when the library containing them is unloaded.
 
+Temporary: A flag variable has been created in error_message.c which
+is used to try to catch cases where remove_error_table is called after
+the library finalization function.  This generally indicates
+out-of-order execution of the library finalization functions.  The
+handling of this flag is not thread-safe, but if the finalization
+function is called, other threads should in theory be finished with
+this library anyways.
+
+Statics: error_message.c, com_err.c, covered above.
+
 ----------------
 
 libprofile (and its use in libkrb5)
@@ -77,6 +107,9 @@ former should be pretty safe on UNIX with atomic rename, but on
 Windows there's a race condition; there's a window (so to speak) where
 the filename does not correspond to an actual file.
 
+Statics: prof_file.c, a list of opened config files and their parse
+trees, and a mutex to protect it.
+
 ----------------
 
 libk5crypto
@@ -91,6 +124,12 @@ multi-threaded program?
 
 Debug var in pbkdf2.c.
 
+Statics: pbkdf2.c: debug variable.
+
+Statics: prng.c: Global Yarrow data and mutex.
+
+Statics: crypto_libinit.c: library initializer aux data.
+
 ----------------
 
 libkrb5
@@ -99,26 +138,52 @@ libkrb5
 
 Uses: ctype macros
 
-Uses: res_search, dn_expand, getaddrinfo, getservbyname, getnameinfo
-
-According to current specifications, getaddrinfo should be
-thread-safe; some implementations are not, and we're not attempting to
-figure out which ones.
+Uses: getaddrinfo, getnameinfo.  According to current specifications,
+getaddrinfo should be thread-safe; some implementations are not, and
+we're not attempting to figure out which ones.
 
-Uses: getpwname, getpwuid -- should use _r versions if available
-
-Uses: gmtime, localtime -- should use _r versions
+Uses: res_ninit, res_nsearch.  If these aren't available, the non-'n'
+versions will be used, and they are not thread-safe.
 
 Uses: mkstemp, mktemp -- Are these, or our uses of them, likely to be
 thread-safe?
 
-Uses: regcomp, regexec
-
 Uses: sigaction
 
 The use of sigaction is in the code prompting for a password; we try
 to catch the keyboard interrupt character being used and turn it into
-an error return from that function.
+an error return from that function.  THIS IS NOT THREAD-SAFE.
+
+Uses: tcgetattr, tcsetattr.  This is also in the password-prompting
+code.  These are fine as long as no other threads are accessing the
+same terminal at the same time.
+
+Statics: prompter.c: interrupt flag
+
+Statics: ccdefops.c: default operations table pointer
+
+Statics: ktdefname.c: variable to override default keytab name, NEEDS LOCKING
+
+Statics: conv_creds.c: debug variable
+
+Statics: sendto_kdc.c: debug variable, in export list for KDC
+
+Statics: parse.c: default realm cache, NOT THREAD SAFE
+
+Statics: krb5_libinit.c: lib init aux data
+
+Statics: osconfig.c: various internal variables, probably should be const
+
+Statics: init_ctx.c: "brand" string; not written.
+
+Statics: cc_memory.c: list of caches, with mutex.
+
+Statics: c_ustime.c: last timestamp, to implement "microseconds must
+always increment"
+
+Statics: ktbase.c, ccbase.c, rc_base.c: type registries and mutexes.
+
+Needs work: keytab locking
 
 ----------------
 
@@ -128,9 +193,28 @@ libgssapi_krb5
 
 Uses: ctype macros
 
-Uses: getpwuid
+Statics: acquire_cred.c: name of keytab to use, and mutex.
+
+Statics: gssapi_krb5.c:
+
+Statics: init_sec_context.c:
+
+Statics: set_ccache.c:
+
+Statics: gssapi_generic.c: OID definitions, non-const by
+specification.  We probably could make them const anyways.
+
+The keytab name saved away by krb5_gss_register_acceptor_identity is
+global and protected by a mutex; the ccache name stored by
+gss_krb5_ccache_name is per-thread.  This inconsistency is due to the
+anticipated usage patterns.
+
+The old ccache name returned by gss_krb5_ccache_name if the last
+parameter is not a null pointer is also stored per-thread, and will be
+discarded at the next call to that routine from the same thread, or at
+thread termination.
 
-Some static data.
+Needs work: check various objects for thread safety
 
 ----------------
 
@@ -146,7 +230,7 @@ calls.
 
 libgssrpc
 
-Skip this.  We're replacing it anyways.
+New version is in place.  Ignore it for now?
 
 ----------------
 
diff --git a/doc/threads.txt b/doc/threads.txt
index 200fefa..1b655ea 100644
--- a/doc/threads.txt
+++ b/doc/threads.txt
@@ -21,7 +21,7 @@ means, within the krb5 library.)
    //   POSIX: partial initializer is PTHREAD_MUTEX_INITIALIZER,
    //          finish does nothing
    //   Windows: partial initializer is zero/empty,
-   //            finish does the actual work
+   //            finish does the actual work and runs at load time
    //   debug: partial initializer sets one magic value,
    //          finish verifies, sets a new magic value
    k5_mutex_t foo_mutex = K5_MUTEX_PARTIAL_INITIALIZER;
@@ -46,6 +46,9 @@ means, within the krb5 library.)
    // 
    int k5_mutex_lock(k5_mutex_t *);
    int k5_mutex_unlock(k5_mutex_t *);
+   // Optional (always defined, but need not do anything):
+   void k5_mutex_assert_locked(k5_mutex_t *);
+   void k5_mutex_assert_unlocked(k5_mutex_t *);
 
 
    k5_key_t key;
@@ -54,4 +57,25 @@ means, within the krb5 library.)
    int k5_setspecific(k5_key_t, const void *);
    ... stuff to signal library termination ...
 
+
+On many platforms with weak reference support, we can declare certain
+symbols to be weak, and test the addresses before calling them.  The
+references generally will be non-null if the application pulls in the
+pthread support.  Sometimes stubs are present in the C library for
+some of these routines, and sometimes they're not functional; if so,
+we need to figure out which ones, and check for the presence of some
+*other* routines.
+
+AIX 4.3.3 doesn't support weak references.  However, it looks like
+calling dlsym(NULL) causes the pthread library to get loaded, so we're
+going to just go ahead and link against it anyways.
+
+
+For now, the basic model is:
+
+  If weak references supported, use them.
+  Else, assume support is present.
+
+
+
 See also notes in src/include/k5-thread.h.