[pacman-dev] [PATCH] check for backup file changes more cautiously

[Andrew Gregory]

A couple of our tests checked for a non-match between hashes, therefore
assuming that backup files were the same if the hashing process failed.
Because treating the files as the same prevents the user from ever
seeing the new file, we should instead check for a hash match and treat
the files as different unless we have actually been able to verify that
they are the same.

Refactoring also removes the need to set hash_orig = "" just to reach
some of the tests.

Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
---
 lib/libalpm/add.c | 120 ++++++++++++++++++++++++++----------------------------
 1 file changed, 58 insertions(+), 62 deletions(-)

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
index 3ef81e3..9c930f5 100644
--- a/lib/libalpm/add.c
+++ b/lib/libalpm/add.c
@@ -273,8 +273,6 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
 				/* check newpkg first, so that adding backup files is retroactive */
 				backup = _alpm_needbackup(entryname, newpkg);
 				if(backup) {
-					/* if we force hash_orig to be non-NULL retroactive backup works */
-					hash_orig = "";
 					needbackup = 1;
 				}
 
@@ -332,83 +330,81 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
 		_alpm_log(handle, ALPM_LOG_DEBUG, "new:      %s\n", hash_pkg);
 		_alpm_log(handle, ALPM_LOG_DEBUG, "original: %s\n", hash_orig);
 
-		if(!oldpkg) {
-			if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) != 0) {
+		if(hash_orig && hash_local && strcmp(hash_orig, hash_local) == 0) {
+			/* installed file has NOT been changed by user */
+			if(hash_pkg && strcmp(hash_orig, hash_pkg) == 0) {
+				/* no sense in installing the same file twice, install
+				 * ONLY if the original and package hashes differ */
+				_alpm_log(handle, ALPM_LOG_DEBUG,
+						"action: leaving existing file in place\n");
+				unlink(checkfile);
+			} else {
+				_alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",
+						entryname_orig);
+
+				if(try_rename(handle, checkfile, filename)) {
+					errors++;
+				}
+			}
+		} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) {
+			/* originally installed file and new file are the same - this
+			 * implies the case above failed - i.e. the file was changed by a
+			 * user */
+			_alpm_log(handle, ALPM_LOG_DEBUG,
+					"action: leaving existing file in place\n");
+			unlink(checkfile);
+		} else if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) {
+			/* this would be magical.  The above two cases failed, but the
+			 * user changes just so happened to make the new file exactly the
+			 * same as the one in the package... skip it */
+			_alpm_log(handle, ALPM_LOG_DEBUG,
+					"action: leaving existing file in place\n");
+			unlink(checkfile);
+		} else {
+			char *newpath;
+			size_t newlen = strlen(filename) + 9;
+			MALLOC(newpath, newlen,
+					errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
+
+			if(oldpkg) {
+				_alpm_log(handle, ALPM_LOG_DEBUG,
+						"action: keeping current file and installing"
+						" new one with .pacnew ending\n");
+				snprintf(newpath, newlen, "%s.pacnew", filename);
+				if(try_rename(handle, checkfile, newpath)) {
+					errors++;
+				} else {
+					_alpm_log(handle, ALPM_LOG_WARNING, _("%s installed as %s\n"),
+							filename, newpath);
+					alpm_logaction(handle, ALPM_CALLER_PREFIX,
+							"warning: %s installed as %s\n", filename, newpath);
+				}
+			} else {
 				/* looks like we have a local file that has a different hash as the
 				 * file in the package, move it to a .pacorig */
-				char *newpath;
-				size_t newlen = strlen(filename) + 9;
-				MALLOC(newpath, newlen,
-						errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
+				_alpm_log(handle, ALPM_LOG_DEBUG,
+						"action: saving existing file with a .pacorig ending"
+						" and installing a new one\n");
 				snprintf(newpath, newlen, "%s.pacorig", filename);
 
 				/* move the existing file to the "pacorig" */
 				if(try_rename(handle, filename, newpath)) {
-						errors++;
-					errors++;
+					errors++;   /* failed rename filename  -> filename.pacorig */
+					errors++;   /* failed rename checkfile -> filename */
 				} else {
 					/* rename the file we extracted to the real name */
 					if(try_rename(handle, checkfile, filename)) {
 						errors++;
 					} else {
-						_alpm_log(handle, ALPM_LOG_WARNING, _("%s saved as %s\n"), filename, newpath);
+						_alpm_log(handle, ALPM_LOG_WARNING,
+								_("%s saved as %s\n"), filename, newpath);
 						alpm_logaction(handle, ALPM_CALLER_PREFIX,
 								"warning: %s saved as %s\n", filename, newpath);
 					}
 				}
-				free(newpath);
-			} else {
-				/* local file is identical to pkg one, so just remove pkg one */
-				unlink(checkfile);
 			}
-		} else if(hash_orig) {
-			/* the fun part */
-
-			if(hash_local && strcmp(hash_orig, hash_local) == 0) {
-				/* installed file has NOT been changed by user */
-				if(hash_pkg && strcmp(hash_orig, hash_pkg) != 0) {
-					_alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",
-							entryname_orig);
 
-					if(try_rename(handle, checkfile, filename)) {
-						errors++;
-					}
-				} else {
-					/* no sense in installing the same file twice, install
-					 * ONLY if the original and package hashes differ */
-					_alpm_log(handle, ALPM_LOG_DEBUG, "action: leaving existing file in place\n");
-					unlink(checkfile);
-				}
-			} else if(hash_pkg && strcmp(hash_orig, hash_pkg) == 0) {
-				/* originally installed file and new file are the same - this
-				 * implies the case above failed - i.e. the file was changed by a
-				 * user */
-				_alpm_log(handle, ALPM_LOG_DEBUG, "action: leaving existing file in place\n");
-				unlink(checkfile);
-			} else if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) {
-				/* this would be magical.  The above two cases failed, but the
-				 * user changes just so happened to make the new file exactly the
-				 * same as the one in the package... skip it */
-				_alpm_log(handle, ALPM_LOG_DEBUG, "action: leaving existing file in place\n");
-				unlink(checkfile);
-			} else {
-				char *newpath;
-				size_t newlen = strlen(filename) + 8;
-				_alpm_log(handle, ALPM_LOG_DEBUG, "action: keeping current file and installing"
-						" new one with .pacnew ending\n");
-				MALLOC(newpath, newlen,
-						errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
-				snprintf(newpath, newlen, "%s.pacnew", filename);
-				if(try_rename(handle, checkfile, newpath)) {
-					errors++;
-				} else {
-					_alpm_log(handle, ALPM_LOG_WARNING, _("%s installed as %s\n"),
-							filename, newpath);
-					alpm_logaction(handle, ALPM_CALLER_PREFIX,
-							"warning: %s installed as %s\n", filename, newpath);
-				}
-				free(newpath);
-			}
+			free(newpath);
 		}
 
 needbackup_cleanup:
-- 
1.8.2.2