From ac30968b7858d4ca3743d2b4d296eca543864fe2 Mon Sep 17 00:00:00 2001

From: Jiri Vymazal <jvymazal@redhat.com>

Date: Fri, 22 Nov 2019 14:25:59 +0100

Subject: [PATCH] Thorougher state-file renaming and cleaning

Now checking if file-id changes and reanming - cleaning state file

accordingly and always checking and cleaning old inode-only style

state files.

---

 plugins/imfile/imfile.c | 66 +++++++++++++++++++++++++++--------------

 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/plugins/imfile/imfile.c b/plugins/imfile/imfile.c

index d9bf0fbb6d..9db2b47ac9 100644

--- a/plugins/imfile/imfile.c

+++ b/plugins/imfile/imfile.c

@@ -182,6 +182,7 @@ struct act_obj_s {

 	time_t timeoutBase; /* what time to calculate the timeout against? */

 	/* file dynamic data */

 	char file_id[FILE_ID_HASH_SIZE]; /* file id for this entry, once we could obtain it */

+	char file_id_prev[FILE_ID_HASH_SIZE]; /* previous file id for this entry, set if changed */

 	int in_move;	/* workaround for inotify move: if set, state file must not be deleted */

 	ino_t ino;	/* current inode nbr */

 	int fd;		/* fd to file in order to obtain file_id (needs to be preserved across move) */

@@ -711,7 +712,7 @@ act_obj_add(fs_edge_t *const edge, const char *const name, const int is_file,

 		if (is_file) {

 			LogError(errno, RS_RET_ERR, "imfile: error accessing file '%s'", name);

 		} else { /* reporting only in debug for dirs as higher lvl paths are likely blocked by selinux */

-			DBGPRINTF("imfile: error accessing file '%s'", name);

+			DBGPRINTF("imfile: error accessing directory '%s'", name);

 		}

 		FINALIZE;

 	}

@@ -727,6 +728,7 @@ act_obj_add(fs_edge_t *const edge, const char *const name, const int is_file,

 	act->ino = ino;

 	act->fd = fd;

 	act->file_id[0] = '\0';

+	act->file_id_prev[0] = '\0';

 	act->is_symlink = is_symlink;

 	if (source) { /* we are target of symlink */

 		CHKmalloc(act->source_name = strdup(source));

@@ -1256,17 +1258,15 @@ get_file_id_hash(const char *data, size_t lendata,

 static void ATTR_NONNULL(1)

 getFileID(act_obj_t *const act)

 {

-	if(act->file_id[0] != '\0') {

-		return; /* everything already done */

-	}

+	/* save the old id for cleaning purposes */

+	strncpy(act->file_id_prev, (const char*)act->file_id, FILE_ID_HASH_SIZE);

+	act->file_id[0] = '\0';

 	assert(act->fd >= 0); /* fd must have been opened at act_obj_t creation! */

 	char filedata[FILE_ID_SIZE];

+	lseek(act->fd, 0, SEEK_SET); /* Seek to beginning of file so we have correct id */

 	const int r = read(act->fd, filedata, FILE_ID_SIZE);

 	if(r == FILE_ID_SIZE) {

 		get_file_id_hash(filedata, sizeof(filedata), act->file_id, sizeof(act->file_id));

-		dbgprintf("file_id '%s' obtained, closing monitoring file handle\n", act->file_id);

-		close(act->fd); /* we will never go here! */

-		act->fd = -1;

 	} else {

 		DBGPRINTF("getFileID partial or error read, ret %d\n", r);

 	}

@@ -1378,28 +1378,13 @@ openFileWithStateFile(act_obj_t *const act)

 	if(fd < 0) {

 		if(errno == ENOENT) {

 			if(act->file_id[0] != '\0') {

-				const char *pszSFNamHash = strdup((const char*)pszSFNam);

-				CHKmalloc(pszSFNamHash);

 				DBGPRINTF("state file %s for %s does not exist - trying to see if "

 					"inode-only file exists\n", pszSFNam, act->name);

 				getFullStateFileName(statefn, "", pszSFNam, sizeof(pszSFNam));

 				fd = open((char*)pszSFNam, O_CLOEXEC | O_NOCTTY | O_RDONLY, 0600);

 				if(fd >= 0) {

-					dbgprintf("found inode-only state file, renaming it now that we "

-						"know the file_id, new name: %s\n", pszSFNamHash);

-					/* we now can use identify the file, so let's rename it */

-					if(rename((const char*)pszSFNam, pszSFNamHash) != 0) {

-						LogError(errno, RS_RET_IO_ERROR,

-							"imfile error trying to rename state file for '%s' - "

-							"ignoring this error, usually this means a file no "

-							"longer file is left over, but this may also cause "

-							"some real trouble. Still the best we can do ",

-							act->name);

-						free((void*) pszSFNamHash);

-						ABORT_FINALIZE(RS_RET_IO_ERROR);

-					}

+					dbgprintf("found inode-only state file, will be renamed at next persist\n");

 				}

-				free((void*) pszSFNamHash);

 			}

 			if(fd < 0) {

 				DBGPRINTF("state file %s for %s does not exist - trying to see if "

@@ -2609,6 +2594,36 @@ atomicWriteStateFile(const char *fn, const char *content)

 	RETiRet;

 }

+/* This function should be called after any file ID change - that is if

+ * file grown from hash-only statefile, or was truncated, this will ensure

+ * we delete the old file so we do not make garbage in our working dir and

+ * there are no leftover statefiles which can in theory later bind to something

+ * and cause data loss.

+ * jvymazal 2019-11-27

+ */

+static void

+removeOldStatefile(const uchar *statefn, const char *hashToDelete)

+{

+	int ret;

+	uchar statefname[MAXFNAME];

+

+	getFullStateFileName(statefn, hashToDelete, statefname, sizeof(statefname));

+	DBGPRINTF("removing old state file: '%s'\n", statefname);

+	ret = unlink((const char*)statefname);

+	if(ret != 0) {

+		if (errno != ENOENT) {

+			LogError(errno, RS_RET_IO_ERROR,

+				"imfile error trying to delete old state file: '%s' - ignoring this "

+				"error, usually this means a file no longer file is left over, but "

+				"this may also cause some real trouble. Still the best we can do ",

+				statefname);

+		} else {

+			DBGPRINTF("trying to delete no longer valid statefile '%s' which no "

+					  "longer exists (probably already deleted)\n", statefname);

+		}

+	}

+}

+

 /* This function persists information for a specific file being monitored.

  * To do so, it simply persists the stream object. We do NOT abort on error

@@ -2660,6 +2675,11 @@ persistStrmState(act_obj_t *const act)

 	CHKiRet(atomicWriteStateFile((const char*)statefname, jstr));

 	json_object_put(json);

+	/* file-id changed remove the old statefile */

+	if (strncmp((const char *)act->file_id_prev, (const char *)act->file_id, FILE_ID_HASH_SIZE)) {

+		removeOldStatefile(statefn, act->file_id_prev);

+	}

+

 finalize_it:

 	if(iRet != RS_RET_OK) {

 		LogError(0, iRet, "imfile: could not persist state "