From 2d28078f72851c6f111ce50d78331603f1a2011f Mon Sep 17 00:00:00 2001 From: Ken Raeburn Date: Wed, 24 Nov 2004 02:39:44 +0000 Subject: [PATCH] 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 --- src/lib/krb5/keytab/ChangeLog | 15 +++++ src/lib/krb5/keytab/kt_file.c | 111 ++++++++++++++++++++++++++-------- 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 + + * 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 * 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; } -- 2.26.2