fix missing locking in keytab; fix stdio handling too

The keytab type list lock was implemented, but I missed the per-keytab lock.
Since I was in there, I ripped out the bogus stdio buffer mangling that the
code was doing, and set up a buffer to be used that we can sanitize later.

* kt_file.c (struct _krb5_ktfile_data): Add mutex and buffer.
(KTFILEBUFP, KTLOCK, KTUNLOCK, KTCHECKLOCK): New macros.
(krb5_ktfile_resolve): Initialize mutex.
(krb5_ktfile_close): Zap data buffer before freeing.
(krb5_ktfile_get_entry, krb5_ktfile_start_seq_get, krb5_ktfile_get_next,
krb5_ktfile_end_get, krb5_ktfile_add, krb5_ktfile_remove): Lock and unlock the
mutex.
(krb5_ktfileint_open): Check that the mutex is locked.  Set the stdio buffer to
the new buffer in the ktfile data.
(krb5_ktfileint_write_entry, krb5_ktfileint_find_slot): Check that the mutex is
locked.  Don't call setbuf.  Flush the stdio buffer after writing.

ticket: new
target_version: 1.4
tags: pullup

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

2 files changed, 102 insertions, 24 deletions

diff --git a/src/lib/krb5/keytab/ChangeLog b/src/lib/krb5/keytab/ChangeLog
index cf203ff84..7e8186c6e 100644
--- a/src/lib/krb5/keytab/ChangeLog
+++ b/src/lib/krb5/keytab/ChangeLog
@@ -1,3 +1,18 @@
+2004-11-23  Ken Raeburn  <raeburn@mit.edu>
+
+	* kt_file.c (struct _krb5_ktfile_data): Add mutex and buffer.
+	(KTFILEBUFP, KTLOCK, KTUNLOCK, KTCHECKLOCK): New macros.
+	(krb5_ktfile_resolve): Initialize mutex.
+	(krb5_ktfile_close): Zap data buffer before freeing.
+	(krb5_ktfile_get_entry, krb5_ktfile_start_seq_get,
+	krb5_ktfile_get_next, krb5_ktfile_end_get, krb5_ktfile_add,
+	krb5_ktfile_remove): Lock and unlock the mutex.
+	(krb5_ktfileint_open): Check that the mutex is locked.  Set the
+	stdio buffer to the new buffer in the ktfile data.
+	(krb5_ktfileint_write_entry, krb5_ktfileint_find_slot): Check that
+	the mutex is locked.  Don't call setbuf.  Flush the stdio buffer
+	after writing.
+
 2004-11-23  Tom Yu  <tlyu@mit.edu>
 
 	* kt_file.c (krb5_ktfileint_open): Update previous change by
diff --git a/src/lib/krb5/keytab/kt_file.c b/src/lib/krb5/keytab/kt_file.c
index 580302e50..fc33af54a 100644
--- a/src/lib/krb5/keytab/kt_file.c
+++ b/src/lib/krb5/keytab/kt_file.c
@@ -52,7 +52,9 @@
 typedef struct _krb5_ktfile_data {
     char *name;			/* Name of the file */
     FILE *openf;		/* open file, if any. */
+    char iobuf[BUFSIZ];		/* so we can zap it later */
     int	version;		/* Version number of keytab */
+    k5_mutex_t lock;		/* Protect openf, version */
 } krb5_ktfile_data;
 
 /*
@@ -61,7 +63,11 @@ typedef struct _krb5_ktfile_data {
 #define KTPRIVATE(id) ((krb5_ktfile_data *)(id)->data)
 #define KTFILENAME(id) (((krb5_ktfile_data *)(id)->data)->name)
 #define KTFILEP(id) (((krb5_ktfile_data *)(id)->data)->openf)
+#define KTFILEBUFP(id) (((krb5_ktfile_data *)(id)->data)->iobuf)
 #define KTVERSION(id) (((krb5_ktfile_data *)(id)->data)->version)
+#define KTLOCK(id) k5_mutex_lock(&((krb5_ktfile_data *)(id)->data)->lock)
+#define KTUNLOCK(id) k5_mutex_unlock(&((krb5_ktfile_data *)(id)->data)->lock)
+#define KTCHECKLOCK(id) k5_mutex_assert_locked(&((krb5_ktfile_data *)(id)->data)->lock)
 
 extern const struct _krb5_kt_ops krb5_ktf_ops;
 extern const struct _krb5_kt_ops krb5_ktf_writable_ops;
@@ -175,6 +181,7 @@ krb5_error_code KRB5_CALLCONV
 krb5_ktfile_resolve(krb5_context context, const char *name, krb5_keytab *id)
 {
     krb5_ktfile_data *data;
+    krb5_error_code err;
 
     if ((*id = (krb5_keytab) malloc(sizeof(**id))) == NULL)
 	return(ENOMEM);
@@ -185,7 +192,14 @@ krb5_ktfile_resolve(krb5_context context, const char *name, krb5_keytab *id)
 	return(ENOMEM);
     }
 
+    err = k5_mutex_init(&data->lock);
+    if (err) {
+	krb5_xfree(*id);
+	return err;
+    }
+
     if ((data->name = (char *)calloc(strlen(name) + 1, sizeof(char))) == NULL) {
+	k5_mutex_destroy(&data->lock);
 	krb5_xfree(data);
 	krb5_xfree(*id);
 	return(ENOMEM);
@@ -217,6 +231,8 @@ krb5_ktfile_close(krb5_context context, krb5_keytab id)
    */
 {
     krb5_xfree(KTFILENAME(id));
+    zap(KTFILEBUFP(id), BUFSIZ);
+    k5_mutex_destroy(&((krb5_ktfile_data *)id->data)->lock);
     krb5_xfree(id->data);
     id->ops = 0;
     krb5_xfree(id);
@@ -230,7 +246,9 @@ krb5_ktfile_close(krb5_context context, krb5_keytab id)
  */
 
 krb5_error_code KRB5_CALLCONV
-krb5_ktfile_get_entry(krb5_context context, krb5_keytab id, krb5_const_principal principal, krb5_kvno kvno, krb5_enctype enctype, krb5_keytab_entry *entry)
+krb5_ktfile_get_entry(krb5_context context, krb5_keytab id,
+		      krb5_const_principal principal, krb5_kvno kvno,
+		      krb5_enctype enctype, krb5_keytab_entry *entry)
 {
     krb5_keytab_entry cur_entry, new_entry;
     krb5_error_code kerror = 0;
@@ -238,9 +256,15 @@ krb5_ktfile_get_entry(krb5_context context, krb5_keytab id, krb5_const_principal
     krb5_boolean similar;
     int kvno_offset = 0;
 
+    kerror = KTLOCK(id);
+    if (kerror)
+	return kerror;
+
     /* Open the keyfile for reading */
-    if ((kerror = krb5_ktfileint_openr(context, id)))
+    if ((kerror = krb5_ktfileint_openr(context, id))) {
+	KTUNLOCK(id);
 	return(kerror);
+    }
     
     /* 
      * For efficiency and simplicity, we'll use a while true that 
@@ -347,13 +371,16 @@ krb5_ktfile_get_entry(krb5_context context, krb5_keytab id, krb5_const_principal
     }
     if (kerror) {
 	(void) krb5_ktfileint_close(context, id);
+	KTUNLOCK(id);
 	krb5_kt_free_entry(context, &cur_entry);
 	return kerror;
     }
     if ((kerror = krb5_ktfileint_close(context, id)) != 0) {
+	KTUNLOCK(id);
 	krb5_kt_free_entry(context, &cur_entry);
 	return kerror;
     }
+    KTUNLOCK(id);
     *entry = cur_entry;
     return 0;
 }
@@ -399,15 +426,23 @@ krb5_ktfile_start_seq_get(krb5_context context, krb5_keytab id, krb5_kt_cursor *
     krb5_error_code retval;
     long *fileoff;
 
-    if ((retval = krb5_ktfileint_openr(context, id)))
+    retval = KTLOCK(id);
+    if (retval)
+	return retval;
+
+    if ((retval = krb5_ktfileint_openr(context, id))) {
+	KTUNLOCK(id);
 	return retval;
+    }
 
     if (!(fileoff = (long *)malloc(sizeof(*fileoff)))) {
 	krb5_ktfileint_close(context, id);
+	KTUNLOCK(id);
 	return ENOMEM;
     }
     *fileoff = ftell(KTFILEP(id));
     *cursorp = (krb5_kt_cursor)fileoff;
+    KTUNLOCK(id);
 
     return 0;
 }
@@ -423,12 +458,20 @@ krb5_ktfile_get_next(krb5_context context, krb5_keytab id, krb5_keytab_entry *en
     krb5_keytab_entry cur_entry;
     krb5_error_code kerror;
 
-    if (fseek(KTFILEP(id), *fileoff, 0) == -1)
+    kerror = KTLOCK(id);
+    if (kerror)
+	return kerror;
+    if (fseek(KTFILEP(id), *fileoff, 0) == -1) {
+	KTUNLOCK(id);
 	return KRB5_KT_END;
-    if ((kerror = krb5_ktfileint_read_entry(context, id, &cur_entry)))
+    }
+    if ((kerror = krb5_ktfileint_read_entry(context, id, &cur_entry))) {
+	KTUNLOCK(id);
 	return kerror;
+    }
     *fileoff = ftell(KTFILEP(id));
     *entry = cur_entry;
+    KTUNLOCK(id);
     return 0;
 }
 
@@ -439,8 +482,13 @@ krb5_ktfile_get_next(krb5_context context, krb5_keytab id, krb5_keytab_entry *en
 krb5_error_code KRB5_CALLCONV 
 krb5_ktfile_end_get(krb5_context context, krb5_keytab id, krb5_kt_cursor *cursor)
 {
+    krb5_error_code kerror;
+
     krb5_xfree(*cursor);
-    return krb5_ktfileint_close(context, id);
+    KTLOCK(id);
+    kerror = krb5_ktfileint_close(context, id);
+    KTUNLOCK(id);
+    return kerror;
 }
 
 /*
@@ -780,12 +828,20 @@ krb5_ktfile_add(krb5_context context, krb5_keytab id, krb5_keytab_entry *entry)
 {
     krb5_error_code retval;
 
-    if ((retval = krb5_ktfileint_openw(context, id)))
+    retval = KTLOCK(id);
+    if (retval)
+	return retval;
+    if ((retval = krb5_ktfileint_openw(context, id))) {
+	KTUNLOCK(id);
 	return retval;
-    if (fseek(KTFILEP(id), 0, 2) == -1)
+    }
+    if (fseek(KTFILEP(id), 0, 2) == -1) {
+	KTUNLOCK(id);
 	return KRB5_KT_END;
+    }
     retval = krb5_ktfileint_write_entry(context, id, entry);
     krb5_ktfileint_close(context, id);
+    KTUNLOCK(id);
     return retval;
 }
 
@@ -800,7 +856,12 @@ krb5_ktfile_remove(krb5_context context, krb5_keytab id, krb5_keytab_entry *entr
     krb5_error_code     kerror;
     krb5_int32          delete_point;
 
+    kerror = KTLOCK(id);
+    if (kerror)
+	return kerror;
+
     if ((kerror = krb5_ktfileint_openw(context, id))) {
+	KTUNLOCK(id);
 	return kerror;
     }
 
@@ -829,6 +890,7 @@ krb5_ktfile_remove(krb5_context context, krb5_keytab id, krb5_keytab_entry *entr
 
     if (kerror) {
 	(void) krb5_ktfileint_close(context, id);
+	KTUNLOCK(id);
 	return kerror;
     }
 
@@ -839,7 +901,7 @@ krb5_ktfile_remove(krb5_context context, krb5_keytab id, krb5_keytab_entry *entr
     } else {
         kerror = krb5_ktfileint_close(context, id);
     }
-
+    KTUNLOCK(id);
     return kerror;
 }
 
@@ -999,6 +1061,7 @@ krb5_ktfileint_open(krb5_context context, krb5_keytab id, int mode)
     krb5_kt_vno kt_vno;
     int writevno = 0;
 
+    KTCHECKLOCK(id);
     errno = 0;
     KTFILEP(id) = fopen(KTFILENAME(id),
 			(mode == KRB5_LOCKMODE_EXCLUSIVE) ?
@@ -1021,7 +1084,7 @@ krb5_ktfileint_open(krb5_context context, krb5_keytab id, int mode)
 	return kerror;
     }
     /* assume ANSI or BSD-style stdio */
-    setbuf(KTFILEP(id), NULL);
+    setbuf(KTFILEP(id), KTFILEBUFP(id));
 
     /* get the vno and verify it */
     if (writevno) {
@@ -1069,6 +1132,7 @@ krb5_ktfileint_close(krb5_context context, krb5_keytab id)
 {
     krb5_error_code kerror;
 
+    KTCHECKLOCK(id);
     if (!KTFILEP(id))
 	return 0;
     kerror = krb5_unlock_file(context, fileno(KTFILEP(id)));
@@ -1084,6 +1148,7 @@ krb5_ktfileint_delete_entry(krb5_context context, krb5_keytab id, krb5_int32 del
     krb5_int32  len;
     char        iobuf[BUFSIZ];
 
+    KTCHECKLOCK(id);
     if (fseek(KTFILEP(id), delete_point, SEEK_SET)) {
         return errno;
     }
@@ -1142,6 +1207,7 @@ krb5_ktfileint_internal_read_entry(krb5_context context, krb5_keytab id, krb5_ke
     char	*tmpdata;
     krb5_data	*princ;
 
+    KTCHECKLOCK(id);
     memset(ret_entry, 0, sizeof(krb5_keytab_entry));
     ret_entry->magic = KV5M_KEYTAB_ENTRY;
 
@@ -1358,8 +1424,8 @@ krb5_ktfileint_write_entry(krb5_context context, krb5_keytab id, krb5_keytab_ent
     krb5_int32  size_needed;
     krb5_int32  commit_point;
     int		i;
-    char iobuf[BUFSIZ];
 
+    KTCHECKLOCK(id);
     retval = krb5_ktfileint_size_entry(context, entry, &size_needed);
     if (retval)
         return retval;
@@ -1367,10 +1433,8 @@ krb5_ktfileint_write_entry(krb5_context context, krb5_keytab id, krb5_keytab_ent
     if (retval)
         return retval;
 
-    setbuf(KTFILEP(id), iobuf);
-
     /* fseek to synchronise buffered I/O on the key table. */
-
+    /* XXX Without the weird setbuf crock, can we get rid of this now?  */
     if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
     {
         return errno;
@@ -1384,7 +1448,6 @@ krb5_ktfileint_write_entry(krb5_context context, krb5_keytab id, krb5_keytab_ent
     
     if (!xfwrite(&count, sizeof(count), 1, KTFILEP(id))) {
     abend:
-	setbuf(KTFILEP(id), 0);
 	return KRB5_KT_IOERR;
     }
     size = krb5_princ_realm(context, entry->principal)->length;
@@ -1459,14 +1522,13 @@ krb5_ktfileint_write_entry(krb5_context context, krb5_keytab id, krb5_keytab_ent
     }
     if (!xfwrite(entry->key.contents, sizeof(krb5_octet),
 		 entry->key.length, KTFILEP(id))) {
-	memset(iobuf, 0, sizeof(iobuf));
-	setbuf(KTFILEP(id), 0);
-	return KRB5_KT_IOERR;
+	goto abend;
     }	
 
+    if (fflush(KTFILEP(id)))
+	goto abend;
+
     retval = krb5_sync_disk_file(context, KTFILEP(id));
-    (void) memset(iobuf, 0, sizeof(iobuf));
-    setbuf(KTFILEP(id), 0);
 
     if (retval) {
         return retval;
@@ -1480,6 +1542,8 @@ krb5_ktfileint_write_entry(krb5_context context, krb5_keytab id, krb5_keytab_ent
     if (!xfwrite(&size_needed, sizeof(size_needed), 1, KTFILEP(id))) {
         goto abend;
     }
+    if (fflush(KTFILEP(id)))
+	goto abend;
     retval = krb5_sync_disk_file(context, KTFILEP(id));
 
     return retval;
@@ -1538,6 +1602,7 @@ krb5_ktfileint_find_slot(krb5_context context, krb5_keytab id, krb5_int32 *size_
     krb5_boolean    found = FALSE;
     char            iobuf[BUFSIZ];
 
+    KTCHECKLOCK(id);
     /*
      * Skip over file version number
      */
@@ -1554,11 +1619,10 @@ krb5_ktfileint_find_slot(krb5_context context, krb5_keytab id, krb5_int32 *size_
             /*
              * Hit the end of file, reserve this slot.
              */
-            setbuf(KTFILEP(id), 0);
             size = 0;
 
             /* fseek to synchronise buffered I/O on the key table. */
-
+	    /* XXX Without the weird setbuf hack, can we nuke this now?  */
             if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
             {
                 return errno;
@@ -1609,7 +1673,6 @@ krb5_ktfileint_find_slot(krb5_context context, krb5_keytab id, krb5_int32 *size_
                  * Make sure we zero any trailing data.
                  */
                 zero_point = ftell(KTFILEP(id));
-                setbuf(KTFILEP(id), iobuf);
                 while ((size = xfread(iobuf, 1, sizeof(iobuf), KTFILEP(id)))) {
                     if (size != sizeof(iobuf)) {
                         remainder = size % sizeof(krb5_int32);
@@ -1625,6 +1688,7 @@ krb5_ktfileint_find_slot(krb5_context context, krb5_keytab id, krb5_int32 *size_
 
                     memset(iobuf, 0, (size_t) size);
                     xfwrite(iobuf, 1, (size_t) size, KTFILEP(id));
+		    fflush(KTFILEP(id));
                     if (feof(KTFILEP(id))) {
                         break;
                     }
@@ -1635,7 +1699,6 @@ krb5_ktfileint_find_slot(krb5_context context, krb5_keytab id, krb5_int32 *size_
                     }
 
                 }
-                setbuf(KTFILEP(id), 0);
                 if (fseek(KTFILEP(id), zero_point, SEEK_SET)) {
                     return errno;
                 }