From 621d74fd6a119f1c68b9c79aa6cdd7efbc9a0e6d Mon Sep 17 00:00:00 2001 From: Norbert Heusser Date: Fri, 10 Jun 2022 17:12:03 +0000 Subject: [PATCH] Created new helper function mosquitto_write_file in common/misc_mosq.h to consolidate saving config files in failsafe manner Signed-off-by: Norbert Heusser --- apps/mosquitto_ctrl/Makefile | 3 - common/misc_mosq.c | 132 ++++++++++++++++++++++++++++++ common/misc_mosq.h | 2 + plugins/dynamic-security/config.c | 49 +++++------ src/persist_write.c | 118 +++++--------------------- 5 files changed, 176 insertions(+), 128 deletions(-) diff --git a/apps/mosquitto_ctrl/Makefile b/apps/mosquitto_ctrl/Makefile index bcf1ef43..ea3e48ea 100644 --- a/apps/mosquitto_ctrl/Makefile +++ b/apps/mosquitto_ctrl/Makefile @@ -96,9 +96,6 @@ memory_public.o : ${R}/src/memory_public.c options.o : options.c mosquitto_ctrl.h ${CROSS_COMPILE}${CC} $(LOCAL_CPPFLAGS) $(APP_CPPFLAGS) $(APP_CFLAGS) -c $< -o $@ -misc_mosq.o : ${R}/lib/misc_mosq.c ${R}/lib/misc_mosq.h - ${CROSS_COMPILE}${CC} $(APP_CPPFLAGS) $(APP_CFLAGS) -c $< -o $@ - password_mosq.o : ${R}/common/password_mosq.c ${R}/common/password_mosq.h ${CROSS_COMPILE}${CC} $(APP_CPPFLAGS) $(APP_CFLAGS) -c $< -o $@ diff --git a/common/misc_mosq.c b/common/misc_mosq.c index d8234086..5e97bf42 100644 --- a/common/misc_mosq.c +++ b/common/misc_mosq.c @@ -22,10 +22,13 @@ Contributors: #include "config.h" #include +#include +#include #include #include #include #include +#include #ifdef WIN32 # include @@ -210,3 +213,132 @@ char *fgets_extending(char **buf, int *buflen, FILE *stream) *buf = newbuf; }while(1); } + + +#include + +#define INVOKE_LOG_FN(format,...) \ + do { \ + if (log_fn){ \ + int tmp_err_no = errno; \ + char msg[2*PATH_MAX]; \ + snprintf(msg, sizeof(msg), (format), __VA_ARGS__); \ + msg[sizeof(msg)-1] = '\0'; \ + (*log_fn)(msg); \ + errno = tmp_err_no; \ + } \ + } while (0) + +int mosquitto_write_file(const char* target_path, bool restrict_read, int (*write_fn)(FILE* fptr, void* user_data), void* user_data, void (*log_fn)(const char* msg)) +{ + int rc = 0; + FILE *fptr = NULL; + char tmp_file_path[PATH_MAX]; + + if (!target_path || !write_fn){ + return MOSQ_ERR_INVAL; + } + + rc = snprintf(tmp_file_path, PATH_MAX, "%s.new", target_path); + if (rc < 0 || rc >= PATH_MAX){ + return MOSQ_ERR_INVAL; + } + +#ifndef WIN32 + /** + * + * If a system lost power during the rename operation at the + * end of this file the filesystem could potentially be left + * with a directory that looks like this after powerup: + * + * 24094 -rw-r--r-- 2 root root 4099 May 30 16:27 mosquitto.db + * 24094 -rw-r--r-- 2 root root 4099 May 30 16:27 mosquitto.db.new + * + * The 24094 shows that mosquitto.db.new is hard-linked to the + * same file as mosquitto.db. If fopen(outfile, "wb") is naively + * called then mosquitto.db will be truncated and the database + * potentially corrupted. + * + * Any existing mosquitto.db.new file must be removed prior to + * opening to guarantee that it is not hard-linked to + * mosquitto.db. + * + */ + if (unlink(tmp_file_path) && errno != ENOENT){ + INVOKE_LOG_FN("unable to remove stale tmp file %s, error %s",tmp_file_path,strerror(errno)); + return MOSQ_ERR_INVAL; + } +#endif + + fptr = mosquitto__fopen(tmp_file_path, "wb", restrict_read); + if(fptr == NULL){ + INVOKE_LOG_FN("unable to open %s for writing, error %s",tmp_file_path,strerror(errno)); + return MOSQ_ERR_INVAL; + } + + if ((rc = (*write_fn)(fptr,user_data)) != MOSQ_ERR_SUCCESS){ + goto error; + } + + rc = MOSQ_ERR_ERRNO; +#ifndef WIN32 + /** + * + * Closing a file does not guarantee that the contents are + * written to disk. Need to flush to send data from app to OS + * buffers, then fsync to deliver data from OS buffers to disk + * (as well as disk hardware permits). + * + * man close (http://linux.die.net/man/2/close, 2016-06-20): + * + * "successful close does not guarantee that the data has + * been successfully saved to disk, as the kernel defers + * writes. It is not common for a filesystem to flush + * the buffers when the stream is closed. If you need + * to be sure that the data is physically stored, use + * fsync(2). (It will depend on the disk hardware at this + * point." + * + * This guarantees that the new state file will not overwrite + * the old state file before its contents are valid. + * + */ + if (fflush(fptr) != 0 && errno != EINTR){ + INVOKE_LOG_FN("unable to flush %s, error %s",tmp_file_path,strerror(errno)); + goto error; + } + + if (fsync(fileno(fptr)) != 0 && errno != EINTR){ + INVOKE_LOG_FN("unable to sync %s to disk, error %s",tmp_file_path,strerror(errno)); + goto error; + } +#endif + + if (fclose(fptr) != 0){ + INVOKE_LOG_FN("unable to close %s on disk, error %s",tmp_file_path,strerror(errno)); + fptr = NULL; + goto error; + } + +#ifdef WIN32 + if(remove(target_path) != 0 && errno != ENOENT){ + INVOKE_LOG_FN("unable to remove %s on disk, error %s",target_path,strerror(errno)); + goto error; + } +#endif + + if(rename(tmp_file_path, target_path) != 0){ + INVOKE_LOG_FN("unable to replace %s by tmp file %s, error %s",target_path,tmp_file_path,strerror(errno)); + goto error; + } + return MOSQ_ERR_SUCCESS; + +error: + if(fptr){ + fclose(fptr); + unlink(tmp_file_path); + } + return MOSQ_ERR_ERRNO; +} + + diff --git a/common/misc_mosq.h b/common/misc_mosq.h index 371eacfc..8c667a96 100644 --- a/common/misc_mosq.h +++ b/common/misc_mosq.h @@ -25,4 +25,6 @@ FILE *mosquitto__fopen(const char *path, const char *mode, bool restrict_read); char *misc__trimblanks(char *str); char *fgets_extending(char **buf, int *buflen, FILE *stream); +int mosquitto_write_file(const char* target_path, bool restrict_read, int (*write_fn)(FILE* fptr, void* user_data), void* user_data, void (*log_fn)(const char* msg)); + #endif diff --git a/plugins/dynamic-security/config.c b/plugins/dynamic-security/config.c index dd3823a4..fb153c3e 100644 --- a/plugins/dynamic-security/config.c +++ b/plugins/dynamic-security/config.c @@ -26,6 +26,7 @@ Contributors: #include #include "json_help.h" +#include "misc_mosq.h" #include "mosquitto.h" #include "mosquitto_broker.h" #include "mosquitto_plugin.h" @@ -194,45 +195,35 @@ char *dynsec__config_to_json(struct dynsec__data *data) return json_str; } -void dynsec__config_save(struct dynsec__data *data) +void dynsec__log_write_error(const char* msg) { - size_t file_path_len; - char *file_path; - FILE *fptr; - size_t json_str_len; + mosquitto_log_printf(MOSQ_LOG_ERR, "Error saving Dynamic security plugin config: %s", msg); +} + +int dynsec__write_json_config(FILE* fptr, void* user_data) +{ + struct dynsec__data *data = (struct dynsec__data *)user_data; char *json_str; + size_t json_str_len; + int rc = MOSQ_ERR_SUCCESS; json_str = dynsec__config_to_json(data); if(json_str == NULL){ mosquitto_log_printf(MOSQ_LOG_ERR, "Error saving Dynamic security plugin config: Out of memory.\n"); - return; + return MOSQ_ERR_NOMEM; } json_str_len = strlen(json_str); - /* Save to file */ - file_path_len = strlen(data->config_file) + 1; - file_path = mosquitto_malloc(file_path_len); - if(file_path == NULL){ - mosquitto_free(json_str); - mosquitto_log_printf(MOSQ_LOG_ERR, "Error saving Dynamic security plugin config: Out of memory.\n"); - return; - } - snprintf(file_path, file_path_len, "%s.new", data->config_file); + if (fwrite(json_str, 1, json_str_len, fptr) != json_str_len){ + mosquitto_log_printf(MOSQ_LOG_ERR, "Error saving Dynamic security plugin config: Cannot write whole config (%ld) bytes to file %s", json_str_len, data->config_file); + rc = MOSQ_ERR_UNKNOWN; + } - fptr = fopen(file_path, "wt"); - if(fptr == NULL){ - mosquitto_free(json_str); - mosquitto_free(file_path); - mosquitto_log_printf(MOSQ_LOG_ERR, "Error saving Dynamic security plugin config: File is not writable - check permissions.\n"); - return; - } - fwrite(json_str, 1, json_str_len, fptr); mosquitto_free(json_str); - fclose(fptr); + return rc; +} - /* Everything is ok, so move new file over proper file */ - if(rename(file_path, data->config_file) < 0){ - mosquitto_log_printf(MOSQ_LOG_ERR, "Error updating dynsec config file: %s", strerror(errno)); - } - mosquitto_free(file_path); +void dynsec__config_save(struct dynsec__data *data) +{ + mosquitto_write_file(data->config_file, true, &dynsec__write_json_config, data, &dynsec__log_write_error); } diff --git a/src/persist_write.c b/src/persist_write.c index 7f913ffe..eb429782 100644 --- a/src/persist_write.c +++ b/src/persist_write.c @@ -296,67 +296,31 @@ static int persist__retain_save_all(FILE *db_fptr) return MOSQ_ERR_SUCCESS; } -int persist__backup(bool shutdown) +static int persist__write_data(FILE* db_fptr, void* user_data); + +static void persist__log_write_error(const char* msg) { - int rc = 0; - FILE *db_fptr = NULL; - uint32_t db_version_w = htonl(MOSQ_DB_VERSION); - uint32_t crc = 0; - char *err; - char *outfile = NULL; - size_t len; - struct PF_cfg cfg_chunk; + log__printf(NULL, MOSQ_LOG_ERR, "Error saving in-memory database, %s", msg); +} +int persist__backup(bool shutdown) +{ if(db.config == NULL) return MOSQ_ERR_INVAL; if(db.config->persistence == false) return MOSQ_ERR_SUCCESS; if(db.config->persistence_filepath == NULL) return MOSQ_ERR_INVAL; log__printf(NULL, MOSQ_LOG_INFO, "Saving in-memory database to %s.", db.config->persistence_filepath); - len = strlen(db.config->persistence_filepath)+5; - outfile = mosquitto__malloc(len+1); - if(!outfile){ - log__printf(NULL, MOSQ_LOG_INFO, "Error saving in-memory database, out of memory."); - return MOSQ_ERR_NOMEM; - } - snprintf(outfile, len, "%s.new", db.config->persistence_filepath); - outfile[len] = '\0'; - -#ifndef WIN32 - /** - * - * If a system lost power during the rename operation at the - * end of this file the filesystem could potentially be left - * with a directory that looks like this after powerup: - * - * 24094 -rw-r--r-- 2 root root 4099 May 30 16:27 mosquitto.db - * 24094 -rw-r--r-- 2 root root 4099 May 30 16:27 mosquitto.db.new - * - * The 24094 shows that mosquitto.db.new is hard-linked to the - * same file as mosquitto.db. If fopen(outfile, "wb") is naively - * called then mosquitto.db will be truncated and the database - * potentially corrupted. - * - * Any existing mosquitto.db.new file must be removed prior to - * opening to guarantee that it is not hard-linked to - * mosquitto.db. - * - */ - rc = unlink(outfile); - if (rc != 0) { - rc = 0; - if (errno != ENOENT) { - log__printf(NULL, MOSQ_LOG_INFO, "Error saving in-memory database, unable to remove %s.", outfile); - goto error; - } - } -#endif + return mosquitto_write_file(db.config->persistence_filepath,true,&persist__write_data,&shutdown,&persist__log_write_error); +} - db_fptr = mosquitto__fopen(outfile, "wb", true); - if(db_fptr == NULL){ - log__printf(NULL, MOSQ_LOG_INFO, "Error saving in-memory database, unable to open %s for writing.", outfile); - goto error; - } +static int persist__write_data(FILE* db_fptr, void* user_data) +{ + bool shutdown = *(bool*)(user_data); + uint32_t db_version_w = htonl(MOSQ_DB_VERSION); + uint32_t crc = 0; + const char* err; + struct PF_cfg cfg_chunk; /* Header */ write_e(db_fptr, magic, 15); @@ -375,54 +339,16 @@ int persist__backup(bool shutdown) goto error; } - persist__client_save(db_fptr); - persist__subs_save_all(db_fptr); - persist__retain_save_all(db_fptr); - -#ifndef WIN32 - /** - * - * Closing a file does not guarantee that the contents are - * written to disk. Need to flush to send data from app to OS - * buffers, then fsync to deliver data from OS buffers to disk - * (as well as disk hardware permits). - * - * man close (http://linux.die.net/man/2/close, 2016-06-20): - * - * "successful close does not guarantee that the data has - * been successfully saved to disk, as the kernel defers - * writes. It is not common for a filesystem to flush - * the buffers when the stream is closed. If you need - * to be sure that the data is physically stored, use - * fsync(2). (It will depend on the disk hardware at this - * point." - * - * This guarantees that the new state file will not overwrite - * the old state file before its contents are valid. - * - */ - - fflush(db_fptr); - fsync(fileno(db_fptr)); -#endif - fclose(db_fptr); - -#ifdef WIN32 - if(remove(db.config->persistence_filepath) != 0){ - if(errno != ENOENT){ - goto error; - } - } -#endif - if(rename(outfile, db.config->persistence_filepath) != 0){ + if (persist__client_save(db_fptr) + || persist__subs_save_all(db_fptr) + || persist__retain_save_all(db_fptr)){ goto error; } - mosquitto__FREE(outfile); - return rc; + return MOSQ_ERR_SUCCESS; + error: - mosquitto__FREE(outfile); err = strerror(errno); - log__printf(NULL, MOSQ_LOG_ERR, "Error: %s.", err); + log__printf(NULL, MOSQ_LOG_ERR, "Error during saving in-memory database %s: %s.", db.config->persistence_filepath, err); if(db_fptr) fclose(db_fptr); return 1; }