From: Andrew Deason <adeason@sinenomine.net>
Date: Mon, 20 Apr 2020 18:03:15 +0000 (-0500)
Subject: ubik: Avoid unlinking garbage during recovery
X-Git-Tag: openafs-devel-1_9_0~90
X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=98b5ffb52117aefac5afb47b30ce9b87eb2fdebf;hp=ca847ddf35e336a8bc3159ce4b26f0162417bbd5

ubik: Avoid unlinking garbage during recovery

In urecovery_Interact, if any of our operations fail around
calling DISK_GetFile, we will jump to FetchEndCall and eventually
unlink 'pbuffer'. But if we failed before opening our .DB0.TMP file,
the contents of 'pbuffer' will not be initialized yet.

During most iterations of the recovery loop, the contents of 'pbuffer'
will be filled in from previous loops, and it should always stay the
same, so it's not a big problem. But if this is the first iteration of
the loop, the contents of 'pbuffer' may be stack garbage.

Solve this in two ways. To make sure we don't use garbage contents in
'pbuffer', memset the whole thing to zeroes at the beginning of
urecovery_Interact(). And then to make sure we're not reusing
'pbuffer' contents from previous iterations of the loop, also clear
the first character to NUL each time we arrive at this area of the
recovery code. And avoid unlinking anything if pbuffer starts with a
NUL.

Commit 44e80643 (ubik: Avoid unlinking garbage) fixes the same issue,
but only fixed it in the SDISK_SendFile codepath in remote.c.

Change-Id: Ica39e66efa89562068a4be3a14b2d13594b77f6d
Reviewed-on: https://gerrit.openafs.org/14153
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
---

diff --git a/src/ubik/recovery.c b/src/ubik/recovery.c
index 414d1ad..a53aa59 100644
--- a/src/ubik/recovery.c
+++ b/src/ubik/recovery.c
@@ -472,6 +472,8 @@ urecovery_Interact(void *dummy)
     int fd = -1;
     afs_int32 pass;
 
+    memset(pbuffer, 0, sizeof(pbuffer));
+
     opr_threadname_set("recovery");
 
     /* otherwise, begin interaction */
@@ -608,6 +610,8 @@ urecovery_Interact(void *dummy)
 	    /* we don't have the best version; we should fetch it. */
 	    urecovery_AbortAll(ubik_dbase);
 
+	    pbuffer[0] = '\0';
+
 	    /* Rx code to do the Bulk fetch */
 	    file = 0;
 	    offset = 0;
@@ -723,7 +727,9 @@ urecovery_Interact(void *dummy)
 #endif
 	    }
 	    if (code) {
-		unlink(pbuffer);
+		if (pbuffer[0] != '\0') {
+		    unlink(pbuffer);
+		}
 		/*
 		 * We will effectively invalidate the old data forever now.
 		 * Unclear if we *should* but we do.