[30248] in CVS-changelog-for-Kerberos-V5
krb5 commit: Improve ulog memory hygiene
daemon@ATHENA.MIT.EDU (Greg Hudson)
Fri Jun 29 22:06:19 2018
Date: Fri, 29 Jun 2018 22:06:13 -0400
From: Greg Hudson <ghudson@mit.edu>
Message-Id: <201806300206.w5U26DCl014000@drugstore.mit.edu>
To: cvs-krb5@mit.edu
Reply-To: krbdev@mit.edu
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: cvs-krb5-bounces@mit.edu
https://github.com/krb5/krb5/commit/7aff2511c0dab5c51b1155ca2952521ffb925fc5
commit 7aff2511c0dab5c51b1155ca2952521ffb925fc5
Author: Greg Hudson <ghudson@mit.edu>
Date: Thu Jun 28 00:15:11 2018 -0400
Improve ulog memory hygiene
Add a helper create_log_context() to initialize a krb5_context's
kdblog_context field, setting ulogfd to -1. Use it in ulog_set_role()
and ulog_map(). In ulog_fini(), release ulogfd if it is not -1.
In ulog_map(), add a cleanup label and use it to finalize the log
context on failure, so that we don't (trivially) leak the mapped ulog.
To reduce the number of "retval = errno;" statements required for this
change, make extend_file_to() return a krb5_error_code.
The ulog leak on error was reported by Bean Zhang.
ticket: 8707
src/lib/kdb/kdb_log.c | 125 +++++++++++++++++++++++++-----------------------
src/slave/kproplog.c | 4 +-
2 files changed, 68 insertions(+), 61 deletions(-)
diff --git a/src/lib/kdb/kdb_log.c b/src/lib/kdb/kdb_log.c
index e48f59a..2b0d633 100644
--- a/src/lib/kdb/kdb_log.c
+++ b/src/lib/kdb/kdb_log.c
@@ -34,6 +34,23 @@ static int pagesize = 0;
ulog = log_ctx->ulog; \
assert(ulog != NULL)
+/* Initialize context->kdblog_context if it does not yet exist, and return it.
+ * Return NULL on allocation failure. */
+static kdb_log_context *
+create_log_context(krb5_context context)
+{
+ kdb_log_context *log_ctx;
+
+ if (context->kdblog_context != NULL)
+ return context->kdblog_context;
+ log_ctx = calloc(1, sizeof(*log_ctx));
+ if (log_ctx == NULL)
+ return NULL;
+ log_ctx->ulogfd = -1;
+ context->kdblog_context = log_ctx;
+ return log_ctx;
+}
+
static inline krb5_boolean
time_equal(const kdbe_time_t *a, const kdbe_time_t *b)
{
@@ -130,7 +147,7 @@ get_sno_status(kdb_log_context *log_ctx, const kdb_last_t *last)
}
/* Extend update log file. */
-static int
+static krb5_error_code
extend_file_to(int fd, unsigned int new_size)
{
off_t current_offset;
@@ -140,22 +157,18 @@ extend_file_to(int fd, unsigned int new_size)
current_offset = lseek(fd, 0, SEEK_END);
if (current_offset < 0)
- return -1;
- if (new_size > INT_MAX) {
- errno = EINVAL;
- return -1;
- }
+ return errno;
+ if (new_size > INT_MAX)
+ return EINVAL;
while (current_offset < (off_t)new_size) {
write_size = new_size - current_offset;
if (write_size > 512)
write_size = 512;
wrote_size = write(fd, zero, write_size);
if (wrote_size < 0)
- return -1;
- if (wrote_size == 0) {
- errno = EINVAL;
- return -1;
- }
+ return errno;
+ if (wrote_size == 0)
+ return EINVAL;
current_offset += wrote_size;
write_size = new_size - current_offset;
}
@@ -194,10 +207,7 @@ resize(kdb_hlog_t *ulog, uint32_t ulogentries, int ulogfd,
sync_header(ulog);
/* Expand log considering new block size. */
- if (extend_file_to(ulogfd, new_size) < 0)
- return errno;
-
- return 0;
+ return extend_file_to(ulogfd, new_size);
}
/* Set the ulog to contain only a dummy entry with the given serial number and
@@ -454,13 +464,7 @@ ulog_init_header(krb5_context context)
return 0;
}
-/*
- * Map the log file to memory for performance and simplicity.
- *
- * Called by: if iprop_enabled then ulog_map();
- * Assumes that the caller will terminate on ulog_map, hence munmap and
- * closing of the fd are implicitly performed by the caller.
- */
+/* Map the log file to memory for performance and simplicity. */
krb5_error_code
ulog_map(krb5_context context, const char *logname, uint32_t ulogentries)
{
@@ -469,50 +473,49 @@ ulog_map(krb5_context context, const char *logname, uint32_t ulogentries)
uint32_t filesize;
kdb_log_context *log_ctx;
kdb_hlog_t *ulog = NULL;
- int ulogfd = -1;
+ krb5_boolean locked = FALSE;
+
+ log_ctx = create_log_context(context);
+ if (log_ctx == NULL)
+ return ENOMEM;
if (stat(logname, &st) == -1) {
- ulogfd = open(logname, O_RDWR | O_CREAT, 0600);
- if (ulogfd == -1)
- return errno;
+ log_ctx->ulogfd = open(logname, O_RDWR | O_CREAT, 0600);
+ if (log_ctx->ulogfd == -1) {
+ retval = errno;
+ goto cleanup;
+ }
filesize = sizeof(kdb_hlog_t) + ulogentries * ULOG_BLOCK;
- if (extend_file_to(ulogfd, filesize) < 0)
- return errno;
+ retval = extend_file_to(log_ctx->ulogfd, filesize);
+ if (retval)
+ goto cleanup;
} else {
- ulogfd = open(logname, O_RDWR, 0600);
- if (ulogfd == -1)
- return errno;
+ log_ctx->ulogfd = open(logname, O_RDWR, 0600);
+ if (log_ctx->ulogfd == -1) {
+ retval = errno;
+ goto cleanup;
+ }
}
- ulog = mmap(0, MAXLOGLEN, PROT_READ | PROT_WRITE, MAP_SHARED, ulogfd, 0);
+ ulog = mmap(0, MAXLOGLEN, PROT_READ | PROT_WRITE, MAP_SHARED,
+ log_ctx->ulogfd, 0);
if (ulog == MAP_FAILED) {
- /* Can't map update log file to memory. */
- close(ulogfd);
- return errno;
- }
-
- if (!context->kdblog_context) {
- log_ctx = k5alloc(sizeof(kdb_log_context), &retval);
- if (log_ctx == NULL)
- return retval;
- memset(log_ctx, 0, sizeof(*log_ctx));
- context->kdblog_context = log_ctx;
- } else {
- log_ctx = context->kdblog_context;
+ retval = errno;
+ goto cleanup;
}
log_ctx->ulog = ulog;
log_ctx->ulogentries = ulogentries;
- log_ctx->ulogfd = ulogfd;
retval = lock_ulog(context, KRB5_LOCKMODE_EXCLUSIVE);
if (retval)
- return retval;
+ goto cleanup;
+ locked = TRUE;
if (ulog->kdb_hmagic != KDB_ULOG_HDR_MAGIC) {
if (ulog->kdb_hmagic != 0) {
- unlock_ulog(context);
- return KRB5_LOG_CORRUPT;
+ retval = KRB5_LOG_CORRUPT;
+ goto cleanup;
}
reset_ulog(log_ctx);
}
@@ -528,14 +531,17 @@ ulog_map(krb5_context context, const char *logname, uint32_t ulogentries)
if (ulog->kdb_num != ulogentries) {
/* Expand the ulog file if it isn't big enough. */
filesize = sizeof(kdb_hlog_t) + ulogentries * ulog->kdb_block;
- if (extend_file_to(ulogfd, filesize) < 0) {
- unlock_ulog(context);
- return errno;
- }
+ retval = extend_file_to(log_ctx->ulogfd, filesize);
+ if (retval)
+ goto cleanup;
}
- unlock_ulog(context);
- return 0;
+cleanup:
+ if (locked)
+ unlock_ulog(context);
+ if (retval)
+ ulog_fini(context);
+ return retval;
}
/* Get the last set of updates seen, (last+1) to n is returned. */
@@ -613,11 +619,8 @@ cleanup:
krb5_error_code
ulog_set_role(krb5_context ctx, iprop_role role)
{
- if (ctx->kdblog_context == NULL) {
- ctx->kdblog_context = calloc(1, sizeof(*ctx->kdblog_context));
- if (ctx->kdblog_context == NULL)
- return ENOMEM;
- }
+ if (create_log_context(ctx) == NULL)
+ return ENOMEM;
ctx->kdblog_context->iproprole = role;
return 0;
}
@@ -678,6 +681,8 @@ ulog_fini(krb5_context context)
return;
if (log_ctx->ulog != NULL)
munmap(log_ctx->ulog, MAXLOGLEN);
+ if (log_ctx->ulogfd != -1)
+ close(log_ctx->ulogfd);
free(log_ctx);
context->kdblog_context = NULL;
}
diff --git a/src/slave/kproplog.c b/src/slave/kproplog.c
index d4aed7b..7e4434e 100644
--- a/src/slave/kproplog.c
+++ b/src/slave/kproplog.c
@@ -494,7 +494,8 @@ main(int argc, char **argv)
exit(1);
}
printf(_("Reinitialized the ulog.\n"));
- exit(0);
+ ulog_fini(context);
+ goto done;
}
ulog = map_ulog(params.iprop_logfile);
@@ -562,6 +563,7 @@ main(int argc, char **argv)
printf("\n");
+done:
kadm5_free_config_params(context, ¶ms);
krb5_free_context(context);
return 0;
_______________________________________________
cvs-krb5 mailing list
cvs-krb5@mit.edu
https://mailman.mit.edu/mailman/listinfo/cvs-krb5
