Merge branch 'fix-corrupted-dynsec-config' of https://github.com/NorbertHeusser/mosquitto into NorbertHeusser-fix-corrupted-dynsec-config

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 $@
 

common/misc_mosq.c

@@ -22,10 +22,13 @@ Contributors:
 #include "config.h"
 
 #include <ctype.h>
+#include <errno.h>
+#include <limits.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 #ifdef WIN32
 #  include <winsock2.h>
@@ -210,3 +213,132 @@ char *fgets_extending(char **buf, int *buflen, FILE *stream)
 		*buf = newbuf;
 	}while(1);
 }
+
+
+#include <string.h>
+
+#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;
+}
+
+

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

plugins/dynamic-security/config.c

@@ -26,6 +26,7 @@ Contributors:
 #include <sys/stat.h>
 
 #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;
+	mosquitto_log_printf(MOSQ_LOG_ERR, "Error saving Dynamic security plugin config: %s", msg);
-	char *file_path;
+}
-	FILE *fptr;
+
-	size_t json_str_len;
+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 */
+	if (fwrite(json_str, 1, json_str_len, fptr) != json_str_len){
-	file_path_len = strlen(data->config_file) + 1;
+		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);
-	file_path = mosquitto_malloc(file_path_len);
+		rc = MOSQ_ERR_UNKNOWN;
-	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);
 
-	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 */
+void dynsec__config_save(struct dynsec__data *data)
-	if(rename(file_path, data->config_file) < 0){
+{
-		mosquitto_log_printf(MOSQ_LOG_ERR, "Error updating dynsec config file: %s", strerror(errno));
+	mosquitto_write_file(data->config_file, true, &dynsec__write_json_config, data, &dynsec__log_write_error);
-	}
-	mosquitto_free(file_path);
 }

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;
+	log__printf(NULL, MOSQ_LOG_ERR, "Error saving in-memory database, %s", msg);
-	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;
 
+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;
+	return mosquitto_write_file(db.config->persistence_filepath,true,&persist__write_data,&shutdown,&persist__log_write_error);
-	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
 
-	db_fptr = mosquitto__fopen(outfile, "wb", true);
+static int persist__write_data(FILE* db_fptr, void* user_data)
-	if(db_fptr == NULL){
+{
-		log__printf(NULL, MOSQ_LOG_INFO, "Error saving in-memory database, unable to open %s for writing.", outfile);
+	bool shutdown = *(bool*)(user_data);
-		goto error;
+	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);
+	if (persist__client_save(db_fptr)
-	persist__subs_save_all(db_fptr);
+			|| persist__subs_save_all(db_fptr)
-	persist__retain_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){
 		goto error;
 	}
-	mosquitto__FREE(outfile);
+	return MOSQ_ERR_SUCCESS;
-	return rc;
+
 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;
 }