Extended attribute improvements (no path, binary data)

Issue #1: If an operation came in for an item with no path
provided by the wrapper, the client would not construct the
combined "path" value. Fixed, and missing paths are now
consistently handled as 0-byte paths.

Issue #2: The database code was assuming the values were
strings, and ignoring a specified length.

Issue #3: The computation of the length of the stored value
was off by one, because it was including the extra terminating
null the client added in case the value was a path.

With this in place, "cp -a" on CentOS is consistently
duplicating the system.posix_acl_access fields as expected,
but unfortunately not handling their permissions too.

(Intent is to translate a system.posix_acl_access setxattr
into corresponding permissions whenever possible.)

Signed-off-by: Peter Seebach <peter.seebach@windriver.com>

3 files changed, 66 insertions, 43 deletions

diff --git a/pseudo.c b/pseudo.c
index bb8c60b..144f441 100644
--- a/pseudo.c
+++ b/pseudo.c
@@ -524,6 +524,7 @@ pseudo_op(pseudo_msg_t *msg, const char *program, const char *tag, char **respon
 	 */
 
 	if (msg->pathlen) {
+		size_t initial_len;
 		switch (msg->op) {
 		case OP_RENAME:
 		case OP_CREATE_XATTR:
@@ -532,10 +533,18 @@ pseudo_op(pseudo_msg_t *msg, const char *program, const char *tag, char **respon
 		case OP_REPLACE_XATTR:
 		case OP_SET_XATTR:
 			/* In a rename there are two paths, null separated in msg->path */
-			oldpath = msg->path + strlen(msg->path) + 1;
-			oldpathlen = msg->pathlen - (oldpath - msg->path);
-			pseudo_debug(PDBGF_OP | PDBGF_FILE | PDBGF_XATTR, "%s: path '%s', oldpath '%s' [%d]\n",
-				pseudo_op_name(msg->op), msg->path, oldpath, (int) oldpathlen);
+			initial_len = strlen(msg->path);
+			oldpath = msg->path + initial_len + 1;
+			/* for rename, the path name would be null-terminated,
+			 * but for *xattr, we don't want the null. */
+			oldpathlen = msg->pathlen - (oldpath - msg->path) - 1;
+			pseudo_debug(PDBGF_OP | PDBGF_FILE | PDBGF_XATTR, "%s: path '%s', oldpath '%s' [%d/%d]\n",
+				pseudo_op_name(msg->op), msg->path, oldpath, (int) oldpathlen, (int) msg->pathlen);
+			/* if we got an oldpath, but a 0-length initial
+			 * path, we don't want to act as though we had
+			 * a non-empty initial path.
+			 */
+			msg->pathlen = initial_len;
 			break;
 		default:
 			break;
@@ -956,8 +965,8 @@ pseudo_op(pseudo_msg_t *msg, const char *program, const char *tag, char **respon
 		} else {
 			*response_path = oldpath;
 			*response_len = oldpathlen;
-			pseudo_debug(PDBGF_XATTR, "get results: '%s' (%d bytes)\n",
-				*response_path, (int) *response_len);
+			pseudo_debug(PDBGF_XATTR, "get results: '%.*s' (%d bytes)\n",
+				(int) *response_len, *response_path, (int) *response_len);
 		}
 		break;
 	case OP_LIST_XATTR:
diff --git a/pseudo_client.c b/pseudo_client.c
index 40985e6..8c39e6b 100644
--- a/pseudo_client.c
+++ b/pseudo_client.c
@@ -1058,6 +1058,7 @@ pseudo_client_op(pseudo_op_t op, int access, int fd, int dirfd, const char *path
 	char *oldpath = 0;
 	size_t oldpathlen = 0;
 	char *alloced_path = 0;
+	int strip_slash;
 
 	/* disable wrappers */
 	pseudo_antimagic();
@@ -1109,7 +1110,20 @@ pseudo_client_op(pseudo_op_t op, int access, int fd, int dirfd, const char *path
 		va_end(ap);
 	}
 
+	/* if path isn't available, try to find one? */
+	if (!path && fd >= 0 && fd <= nfds) {
+		path = fd_path(fd);
+		if (!path) {
+			pathlen = 0;
+		} else {
+			pathlen = strlen(path) + 1;
+		}
+	}
+
 	if (path) {
+		if (pathlen == (size_t) -1) {
+			pathlen = strlen(path) + 1;
+		}
 		/* path fixup has to happen in the specific functions,
 		 * because they may have to make calls which have to be
 		 * fixed up for chroot stuff already.
@@ -1118,43 +1132,43 @@ pseudo_client_op(pseudo_op_t op, int access, int fd, int dirfd, const char *path
 		 * (no attempt is made to handle a rename of "/" occurring
 		 * in a chroot...)
 		 */
-		pathlen = strlen(path) + 1;
-		int strip_slash = (pathlen > 2 && (path[pathlen - 2]) == '/');
-		if (oldpath) {
-			size_t full_len = oldpathlen + 1 + pathlen;
-			size_t partial_len = pathlen - 1 - strip_slash;
-			char *both_paths = malloc(full_len);
-			if (!both_paths) {
-				pseudo_diag("Can't allocate space for paths for a rename operation.  Sorry.\n");
-				pseudo_magic();
-				return 0;
-			}
-			memcpy(both_paths, path, partial_len);
-			both_paths[partial_len] = '\0';
-			memcpy(both_paths + partial_len + 1, oldpath, oldpathlen);
-			both_paths[full_len - 1] = '\0';
-			pseudo_debug(PDBGF_PATH | PDBGF_FILE, "rename: %s -> %s [%d]\n",
-				both_paths + pathlen, both_paths, (int) full_len);
-			alloced_path = both_paths;
-			path = alloced_path;
-			pathlen = full_len;
-		} else {
-			if (strip_slash) {
-				alloced_path = strdup(path);
-				alloced_path[pathlen - 2] = '\0';
-				path = alloced_path;
-			}
+		strip_slash = (pathlen > 2 && (path[pathlen - 2]) == '/');
+	} else {
+		path = "";
+		pathlen = 0;
+		strip_slash = 0;
+	}
+
+	/* f*xattr operations can result in needing to send a path
+	 * value even though we don't have one available. We use an
+	 * empty path for that.
+	 */
+	if (oldpath) {
+		size_t full_len = oldpathlen + 1 + pathlen;
+		size_t partial_len = pathlen - 1 - strip_slash;
+		char *both_paths = malloc(full_len);
+		if (!both_paths) {
+			pseudo_diag("Can't allocate space for paths for a rename operation.  Sorry.\n");
+			pseudo_magic();
+			return 0;
 		}
-	} else if (fd >= 0 && fd <= nfds) {
-		path = fd_path(fd);
-		if (!path)
-			msg.pathlen = 0;
-		else
-			msg.pathlen = strlen(path) + 1;
+		memcpy(both_paths, path, partial_len);
+		both_paths[partial_len] = '\0';
+		memcpy(both_paths + partial_len + 1, oldpath, oldpathlen);
+		both_paths[full_len - 1] = '\0';
+		pseudo_debug(PDBGF_PATH | PDBGF_FILE, "rename: %s -> %s [%d]\n",
+			both_paths + pathlen, both_paths, (int) full_len);
+		alloced_path = both_paths;
+		path = alloced_path;
+		pathlen = full_len;
 	} else {
-		path = 0;
-		msg.pathlen = 0;
+		if (strip_slash) {
+			alloced_path = strdup(path);
+			alloced_path[pathlen - 2] = '\0';
+			path = alloced_path;
+		}
 	}
+
 	pseudo_debug(PDBGF_OP, "%s%s", pseudo_op_name(op),
 		(dirfd != -1 && dirfd != AT_FDCWD && op != OP_DUP) ? "at" : "");
 	if (oldpath) {
diff --git a/pseudo_db.c b/pseudo_db.c
index 2faeb10..0a20b96 100644
--- a/pseudo_db.c
+++ b/pseudo_db.c
@@ -2377,8 +2377,8 @@ pdb_set_xattr(long long file_id, char *value, size_t len, int flags) {
 	vlen = strlen(value);
 	len = len - (vlen + 1);
 	value = value + len;
-	pseudo_debug(PDBGF_XATTR, "trying to set a value for %lld: name is '%s', value is '%s'. Existing row %lld.\n",
-		file_id, vname, value, existing_row);
+	pseudo_debug(PDBGF_XATTR, "trying to set a value for %lld: name is '%s' [%d/%d bytes], value is '%s'. Existing row %lld.\n",
+		file_id, vname, (int) vlen, (int) (len + vlen + 1), value, existing_row);
 	if (existing_row != -1) {
 		/* update */
 		if (!update) {
@@ -2388,7 +2388,7 @@ pdb_set_xattr(long long file_id, char *value, size_t len, int flags) {
 				return 1;
 			}
 		}
-		rc = sqlite3_bind_text(update, 1, value, -1, SQLITE_STATIC);
+		rc = sqlite3_bind_text(update, 1, value, len, SQLITE_STATIC);
 		if (rc) {
 			dberr(file_db, "couldn't bind xattr value to UPDATE.");
 			return 1;