From d9f1be15663f673588e2746e5a97f8ee2c92769f Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Fri, 22 May 2020 10:42:11 -0400 Subject: [PATCH] Issue 51095 - abort operation if CSN can not be generated Bug Description: If we fail to get the system time then we were using an uninitialized timespec struct which could lead to bizarre times in CSN's. Fix description: Check if the system time function fails, and if it does then abort the update operation. relates: https://pagure.io/389-ds-base/issue/51095 Reviewed by: firstyear & tbordaz(Thanks!!) --- ldap/servers/plugins/replication/repl5.h | 2 +- .../plugins/replication/repl5_replica.c | 29 ++++++++----- ldap/servers/slapd/back-ldbm/ldbm_add.c | 8 +++- ldap/servers/slapd/back-ldbm/ldbm_delete.c | 9 +++- ldap/servers/slapd/back-ldbm/ldbm_modify.c | 10 ++++- ldap/servers/slapd/back-ldbm/ldbm_modrdn.c | 8 +++- ldap/servers/slapd/csngen.c | 18 +++++++- ldap/servers/slapd/entrywsi.c | 15 ++++--- ldap/servers/slapd/slap.h | 2 +- ldap/servers/slapd/slapi-plugin.h | 8 ++++ ldap/servers/slapd/slapi-private.h | 5 ++- ldap/servers/slapd/time.c | 43 +++++++++++++------ 12 files changed, 116 insertions(+), 41 deletions(-) diff --git a/ldap/servers/plugins/replication/repl5.h b/ldap/servers/plugins/replication/repl5.h index b06c6fbf4..bdc59422a 100644 --- a/ldap/servers/plugins/replication/repl5.h +++ b/ldap/servers/plugins/replication/repl5.h @@ -775,7 +775,7 @@ void replica_disable_replication(Replica *r, Object *r_obj); int replica_start_agreement(Replica *r, Repl_Agmt *ra); int windows_replica_start_agreement(Replica *r, Repl_Agmt *ra); -CSN *replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn); +int32_t replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn, CSN **opcsn); int replica_get_attr(Slapi_PBlock *pb, const char *type, void *value); /* mapping tree extensions manipulation */ diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c index cdbcde39a..c1b3ed73c 100644 --- a/ldap/servers/plugins/replication/repl5_replica.c +++ b/ldap/servers/plugins/replication/repl5_replica.c @@ -3976,10 +3976,9 @@ windows_replica_start_agreement(Replica *r, Repl_Agmt *ra) * A callback function registered as op->o_csngen_handler and * called by backend ops to generate opcsn. */ -CSN * -replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn) +int32_t +replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn, CSN **opcsn) { - CSN *opcsn = NULL; Object *replica_obj; replica_obj = replica_get_replica_for_op(pb); @@ -3994,17 +3993,25 @@ replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn) CSNGen *gen = (CSNGen *)object_get_data(gen_obj); if (NULL != gen) { /* The new CSN should be greater than the base CSN */ - csngen_new_csn(gen, &opcsn, PR_FALSE /* don't notify */); - if (csn_compare(opcsn, basecsn) <= 0) { + if (csngen_new_csn(gen, opcsn, PR_FALSE /* don't notify */) != CSN_SUCCESS) { + /* Failed to generate CSN we must abort */ + object_release(gen_obj); + return -1; + } + if (csn_compare(*opcsn, basecsn) <= 0) { char opcsnstr[CSN_STRSIZE], basecsnstr[CSN_STRSIZE]; char opcsn2str[CSN_STRSIZE]; - csn_as_string(opcsn, PR_FALSE, opcsnstr); + csn_as_string(*opcsn, PR_FALSE, opcsnstr); csn_as_string(basecsn, PR_FALSE, basecsnstr); - csn_free(&opcsn); + csn_free(opcsn); csngen_adjust_time(gen, basecsn); - csngen_new_csn(gen, &opcsn, PR_FALSE /* don't notify */); - csn_as_string(opcsn, PR_FALSE, opcsn2str); + if (csngen_new_csn(gen, opcsn, PR_FALSE /* don't notify */) != CSN_SUCCESS) { + /* Failed to generate CSN we must abort */ + object_release(gen_obj); + return -1; + } + csn_as_string(*opcsn, PR_FALSE, opcsn2str); slapi_log_err(SLAPI_LOG_WARNING, repl_plugin_name, "replica_generate_next_csn - " "opcsn=%s <= basecsn=%s, adjusted opcsn=%s\n", @@ -4014,7 +4021,7 @@ replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn) * Insert opcsn into the csn pending list. * This is the notify effect in csngen_new_csn(). */ - assign_csn_callback(opcsn, (void *)replica); + assign_csn_callback(*opcsn, (void *)replica); } object_release(gen_obj); } @@ -4023,7 +4030,7 @@ replica_generate_next_csn(Slapi_PBlock *pb, const CSN *basecsn) object_release(replica_obj); } - return opcsn; + return 0; } /* diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index 264f0ceea..09bfb8468 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -644,7 +644,13 @@ ldbm_back_add(Slapi_PBlock *pb) * Current op is a user request. Opcsn will be assigned * if the dn is in an updatable replica. */ - opcsn = entry_assign_operation_csn(pb, e, parententry ? parententry->ep_entry : NULL); + if (entry_assign_operation_csn(pb, e, parententry ? parententry->ep_entry : NULL, &opcsn) != 0) { + slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_add", + "failed to generate add CSN for entry (%s), aborting operation\n", + slapi_entry_get_dn(e)); + ldap_result_code = LDAP_OPERATIONS_ERROR; + goto error_return; + } } if (opcsn != NULL) { entry_set_csn(e, opcsn); diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 1ad846447..6c4229049 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -463,7 +463,14 @@ replace_entry: * by entry_assign_operation_csn() if the dn is in an * updatable replica. */ - opcsn = entry_assign_operation_csn ( pb, e->ep_entry, NULL ); + if (entry_assign_operation_csn(pb, e->ep_entry, NULL, &opcsn) != 0) { + slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_delete", + "failed to generate delete CSN for entry (%s), aborting operation\n", + slapi_entry_get_dn(e->ep_entry)); + retval = -1; + ldap_result_code = LDAP_OPERATIONS_ERROR; + goto error_return; + } } if (opcsn != NULL) { if (!is_fixup_operation) { diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modify.c b/ldap/servers/slapd/back-ldbm/ldbm_modify.c index b0c477e3f..e9d7e87e3 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modify.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modify.c @@ -598,12 +598,18 @@ ldbm_back_modify(Slapi_PBlock *pb) goto error_return; } opcsn = operation_get_csn(operation); - if (NULL == opcsn && operation->o_csngen_handler) { + if (opcsn == NULL && operation->o_csngen_handler) { /* * Current op is a user request. Opcsn will be assigned * if the dn is in an updatable replica. */ - opcsn = entry_assign_operation_csn(pb, e->ep_entry, NULL); + if (entry_assign_operation_csn(pb, e->ep_entry, NULL, &opcsn) != 0) { + slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_modify", + "failed to generate modify CSN for entry (%s), aborting operation\n", + slapi_entry_get_dn(e->ep_entry)); + ldap_result_code = LDAP_OPERATIONS_ERROR; + goto error_return; + } } if (opcsn) { entry_set_maxcsn(e->ep_entry, opcsn); diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c index 26698012a..fde83c99f 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c @@ -543,7 +543,13 @@ ldbm_back_modrdn(Slapi_PBlock *pb) * Current op is a user request. Opcsn will be assigned * if the dn is in an updatable replica. */ - opcsn = entry_assign_operation_csn(pb, e->ep_entry, parententry ? parententry->ep_entry : NULL); + if (entry_assign_operation_csn(pb, e->ep_entry, parententry ? parententry->ep_entry : NULL, &opcsn) != 0) { + slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_modrdn", + "failed to generate modrdn CSN for entry (%s), aborting operation\n", + slapi_entry_get_dn(e->ep_entry)); + ldap_result_code = LDAP_OPERATIONS_ERROR; + goto error_return; + } } if (opcsn != NULL) { entry_set_maxcsn(e->ep_entry, opcsn); diff --git a/ldap/servers/slapd/csngen.c b/ldap/servers/slapd/csngen.c index 68dbbda8e..b08d8b25c 100644 --- a/ldap/servers/slapd/csngen.c +++ b/ldap/servers/slapd/csngen.c @@ -164,6 +164,7 @@ csngen_free(CSNGen **gen) int csngen_new_csn(CSNGen *gen, CSN **csn, PRBool notify) { + struct timespec now = {0}; int rc = CSN_SUCCESS; time_t cur_time; int delta; @@ -179,12 +180,25 @@ csngen_new_csn(CSNGen *gen, CSN **csn, PRBool notify) return CSN_MEMORY_ERROR; } - slapi_rwlock_wrlock(gen->lock); + if ((rc = slapi_clock_gettime(&now)) != 0) { + /* Failed to get system time, we must abort */ + slapi_log_err(SLAPI_LOG_ERR, "csngen_new_csn", + "Failed to get system time (%s)\n", + slapd_system_strerror(rc)); + return CSN_TIME_ERROR; + } + cur_time = now.tv_sec; - cur_time = slapi_current_utc_time(); + slapi_rwlock_wrlock(gen->lock); /* check if the time should be adjusted */ delta = cur_time - gen->state.sampled_time; + if (delta > _SEC_PER_DAY || delta < (-1 * _SEC_PER_DAY)) { + /* We had a jump larger than a day */ + slapi_log_err(SLAPI_LOG_INFO, "csngen_new_csn", + "Detected large jump in CSN time. Delta: %d (current time: %ld vs previous time: %ld)\n", + delta, cur_time, gen->state.sampled_time); + } if (delta > 0) { rc = _csngen_adjust_local_time(gen, cur_time); if (rc != CSN_SUCCESS) { diff --git a/ldap/servers/slapd/entrywsi.c b/ldap/servers/slapd/entrywsi.c index 080eb15aa..12ac5678a 100644 --- a/ldap/servers/slapd/entrywsi.c +++ b/ldap/servers/slapd/entrywsi.c @@ -224,13 +224,12 @@ entry_add_rdn_csn(Slapi_Entry *e, const CSN *csn) slapi_rdn_free(&rdn); } -CSN * -entry_assign_operation_csn(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *parententry) +int32_t +entry_assign_operation_csn(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *parententry, CSN **opcsn) { Slapi_Operation *op; const CSN *basecsn = NULL; const CSN *parententry_dncsn = NULL; - CSN *opcsn = NULL; slapi_pblock_get(pb, SLAPI_OPERATION, &op); @@ -252,14 +251,16 @@ entry_assign_operation_csn(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *parent basecsn = parententry_dncsn; } } - opcsn = op->o_csngen_handler(pb, basecsn); + if(op->o_csngen_handler(pb, basecsn, opcsn) != 0) { + return -1; + } - if (NULL != opcsn) { - operation_set_csn(op, opcsn); + if (*opcsn) { + operation_set_csn(op, *opcsn); } } - return opcsn; + return 0; } /* diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h index 4c53d43dc..1d0a9cbeb 100644 --- a/ldap/servers/slapd/slap.h +++ b/ldap/servers/slapd/slap.h @@ -1464,7 +1464,7 @@ struct op; typedef void (*result_handler)(struct conn *, struct op *, int, char *, char *, int, struct berval **); typedef int (*search_entry_handler)(Slapi_Backend *, struct conn *, struct op *, struct slapi_entry *); typedef int (*search_referral_handler)(Slapi_Backend *, struct conn *, struct op *, struct berval **); -typedef CSN *(*csngen_handler)(Slapi_PBlock *pb, const CSN *basecsn); +typedef int32_t *(*csngen_handler)(Slapi_PBlock *pb, const CSN *basecsn, CSN **opcsn); typedef int (*replica_attr_handler)(Slapi_PBlock *pb, const char *type, void **value); /* diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index 865d83b9b..ea9bcf87b 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -6772,6 +6772,14 @@ int slapi_reslimit_get_integer_limit(Slapi_Connection *conn, int handle, int *li */ time_t slapi_current_time(void) __attribute__((deprecated)); +/** + * Get the system time and check for errors. Return + * + * \param tp - a timespec struct where the system time is set + * \return result code, upon success tp is set to the system time + */ +int32_t slapi_clock_gettime(struct timespec *tp); + /** * Returns the current system time as a hr clock relative to uptime * This means the clock is not affected by timezones diff --git a/ldap/servers/slapd/slapi-private.h b/ldap/servers/slapd/slapi-private.h index d676486a8..f37cfcd41 100644 --- a/ldap/servers/slapd/slapi-private.h +++ b/ldap/servers/slapd/slapi-private.h @@ -227,7 +227,8 @@ enum CSN_INVALID_PARAMETER, /* invalid function argument */ CSN_INVALID_FORMAT, /* invalid state format */ CSN_LDAP_ERROR, /* LDAP operation failed */ - CSN_NSPR_ERROR /* NSPR API failure */ + CSN_NSPR_ERROR, /* NSPR API failure */ + CSN_TIME_ERROR /* Error generating new CSN due to clock failure */ }; typedef struct csngen CSNGen; @@ -320,7 +321,7 @@ int slapi_entries_diff(Slapi_Entry **old_entries, Slapi_Entry **new_entries, int void set_attr_to_protected_list(char *attr, int flag); /* entrywsi.c */ -CSN *entry_assign_operation_csn(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *parententry); +int32_t entry_assign_operation_csn(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *parententry, CSN **opcsn); const CSN *entry_get_maxcsn(const Slapi_Entry *entry); void entry_set_maxcsn(Slapi_Entry *entry, const CSN *csn); const CSN *entry_get_dncsn(const Slapi_Entry *entry); diff --git a/ldap/servers/slapd/time.c b/ldap/servers/slapd/time.c index 8048a3359..545538404 100644 --- a/ldap/servers/slapd/time.c +++ b/ldap/servers/slapd/time.c @@ -61,6 +61,25 @@ poll_current_time() return 0; } +/* + * Check if the time function returns an error. If so return the errno + */ +int32_t +slapi_clock_gettime(struct timespec *tp) +{ + int32_t rc = 0; + + PR_ASSERT(tp && tp->tv_nsec == 0 && tp->tv_sec == 0); + + if (clock_gettime(CLOCK_REALTIME, tp) != 0) { + rc = errno; + } + + PR_ASSERT(rc == 0); + + return rc; +} + time_t current_time(void) { @@ -69,7 +88,7 @@ current_time(void) * but this should be removed in favour of the * more accurately named slapi_current_utc_time */ - struct timespec now; + struct timespec now = {0}; clock_gettime(CLOCK_REALTIME, &now); return now.tv_sec; } @@ -83,7 +102,7 @@ slapi_current_time(void) struct timespec slapi_current_rel_time_hr(void) { - struct timespec now; + struct timespec now = {0}; clock_gettime(CLOCK_MONOTONIC, &now); return now; } @@ -91,7 +110,7 @@ slapi_current_rel_time_hr(void) struct timespec slapi_current_utc_time_hr(void) { - struct timespec ltnow; + struct timespec ltnow = {0}; clock_gettime(CLOCK_REALTIME, <now); return ltnow; } @@ -99,7 +118,7 @@ slapi_current_utc_time_hr(void) time_t slapi_current_utc_time(void) { - struct timespec ltnow; + struct timespec ltnow = {0}; clock_gettime(CLOCK_REALTIME, <now); return ltnow.tv_sec; } @@ -108,8 +127,8 @@ void slapi_timestamp_utc_hr(char *buf, size_t bufsize) { PR_ASSERT(bufsize >= SLAPI_TIMESTAMP_BUFSIZE); - struct timespec ltnow; - struct tm utctm; + struct timespec ltnow = {0}; + struct tm utctm = {0}; clock_gettime(CLOCK_REALTIME, <now); gmtime_r(&(ltnow.tv_sec), &utctm); strftime(buf, bufsize, "%Y%m%d%H%M%SZ", &utctm); @@ -140,7 +159,7 @@ format_localTime_log(time_t t, int initsize __attribute__((unused)), char *buf, { long tz; - struct tm *tmsp, tms; + struct tm *tmsp, tms = {0}; char tbuf[*bufsize]; char sign; /* make sure our buffer will be big enough. Need at least 29 */ @@ -191,7 +210,7 @@ format_localTime_hr_log(time_t t, long nsec, int initsize __attribute__((unused) { long tz; - struct tm *tmsp, tms; + struct tm *tmsp, tms = {0}; char tbuf[*bufsize]; char sign; /* make sure our buffer will be big enough. Need at least 39 */ @@ -278,7 +297,7 @@ slapi_timespec_expire_check(struct timespec *expire) if (expire->tv_sec == 0 && expire->tv_nsec == 0) { return TIMER_CONTINUE; } - struct timespec now; + struct timespec now = {0}; clock_gettime(CLOCK_MONOTONIC, &now); if (now.tv_sec > expire->tv_sec || (expire->tv_sec == now.tv_sec && now.tv_sec > expire->tv_nsec)) { @@ -293,7 +312,7 @@ format_localTime(time_t from) in the syntax of a generalizedTime, except without the time zone. */ { char *into; - struct tm t; + struct tm t = {0}; localtime_r(&from, &t); @@ -362,7 +381,7 @@ format_genTime(time_t from) in the syntax of a generalizedTime. */ { char *into; - struct tm t; + struct tm t = {0}; gmtime_r(&from, &t); into = slapi_ch_malloc(SLAPI_TIMESTAMP_BUFSIZE); @@ -382,7 +401,7 @@ time_t read_genTime(struct berval *from) { struct tm t = {0}; - time_t retTime; + time_t retTime = {0}; time_t diffsec = 0; int i, gflag = 0, havesec = 0; -- 2.26.2