From f4153a493d8bbc371510b5a02eba4afa40b53250 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 22 Jul 2024 18:14:40 +0100 Subject: [PATCH 1/5] Dynamically allocate XFS filesystem names Replace the 256 byte buffer used for names in the XFS filesystem with a dynamically allocated buffer. The define XFS_MAXFILENAMELEN which used to be 255 has been retained, but bumped to 1023. This value is no longer used for long-lived allocations, but is used in chansrv_fuse.c for maintaining state information for in-fligh I/O requests. (cherry picked from commit d8b54357101c6b02b80ee20df8591814689a9ffd) --- sesman/chansrv/chansrv_xfs.c | 65 +++++++++++++++++++++++++++++------- sesman/chansrv/chansrv_xfs.h | 11 ++++-- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/sesman/chansrv/chansrv_xfs.c b/sesman/chansrv/chansrv_xfs.c index 72634df2..1be68daf 100644 --- a/sesman/chansrv/chansrv_xfs.c +++ b/sesman/chansrv/chansrv_xfs.c @@ -264,6 +264,8 @@ xfs_create_xfs_fs(mode_t umask, uid_t uid, gid_t gid) struct xfs_fs *xfs = g_new0(struct xfs_fs, 1); XFS_INODE_ALL *xino1 = NULL; XFS_INODE_ALL *xino2 = NULL; + char *xino1_name = NULL; + char *xino2_name = NULL; if (xfs != NULL) { @@ -279,10 +281,14 @@ xfs_create_xfs_fs(mode_t umask, uid_t uid, gid_t gid) if (!grow_xfs(xfs, INODE_TABLE_ALLOCATION_INITIAL) || xfs->inode_table == NULL || (xino1 = g_new0(XFS_INODE_ALL, 1)) == NULL || - (xino2 = g_new0(XFS_INODE_ALL, 1)) == NULL) + (xino2 = g_new0(XFS_INODE_ALL, 1)) == NULL || + (xino1_name = strdup(".")) == NULL || + (xino2_name = strdup(".delete-pending")) == NULL) { free(xino1); free(xino2); + free(xino1_name); + free(xino2_name); xfs_delete_xfs_fs(xfs); xfs = NULL; } @@ -306,7 +312,7 @@ xfs_create_xfs_fs(mode_t umask, uid_t uid, gid_t gid) xino1->pub.atime = time(0); xino1->pub.mtime = xino1->pub.atime; xino1->pub.ctime = xino1->pub.atime; - strcpy(xino1->pub.name, "."); + xino1->pub.name = xino1_name; xino1->pub.generation = xfs->generation; xino1->pub.is_redirected = 0; xino1->pub.device_id = 0; @@ -328,7 +334,7 @@ xfs_create_xfs_fs(mode_t umask, uid_t uid, gid_t gid) xino2->pub.atime = time(0); xino2->pub.mtime = xino2->pub.atime; xino2->pub.ctime = xino2->pub.atime; - strcpy(xino2->pub.name, ".delete-pending"); + xino2->pub.name = xino2_name; xino2->pub.generation = xfs->generation; xino2->pub.is_redirected = 0; xino2->pub.device_id = 0; @@ -349,6 +355,17 @@ xfs_create_xfs_fs(mode_t umask, uid_t uid, gid_t gid) return xfs; } +/* ------------------------------------------------------------------------ */ +static void +xfs_free_inode(XFS_INODE_ALL *xino) +{ + if (xino != NULL) + { + free(xino->pub.name); + free(xino); + } +} + /* ------------------------------------------------------------------------ */ void xfs_delete_xfs_fs(struct xfs_fs *xfs) @@ -363,7 +380,7 @@ xfs_delete_xfs_fs(struct xfs_fs *xfs) size_t i; for (i = 0 ; i < xfs->inode_count; ++i) { - free(xfs->inode_table[i]); + xfs_free_inode(xfs->inode_table[i]); } } free(xfs->inode_table); @@ -407,9 +424,14 @@ xfs_add_entry(struct xfs_fs *xfs, fuse_ino_t parent_inum, if (xfs->free_count > 0 || grow_xfs(xfs, INODE_TABLE_ALLOCATION_GRANULARITY)) { - XFS_INODE_ALL *xino = NULL; - - if ((xino = g_new0(XFS_INODE_ALL, 1)) != NULL) + XFS_INODE_ALL *xino = g_new0(XFS_INODE_ALL, 1); + char *cpyname = strdup(name); + if (xino == NULL || cpyname == NULL) + { + free(xino); + free(cpyname); + } + else { fuse_ino_t inum = xfs->free_list[--xfs->free_count]; if (xfs->inode_table[inum] != NULL) @@ -433,7 +455,7 @@ xfs_add_entry(struct xfs_fs *xfs, fuse_ino_t parent_inum, xino->pub.atime = time(0); xino->pub.mtime = xino->pub.atime; xino->pub.ctime = xino->pub.atime; - strcpy(xino->pub.name, name); + xino->pub.name = cpyname; xino->pub.generation = xfs->generation; xino->pub.is_redirected = parent->pub.is_redirected; xino->pub.device_id = parent->pub.device_id; @@ -498,7 +520,7 @@ xfs_remove_entry(struct xfs_fs *xfs, fuse_ino_t inum) * so that the caller can distinguish re-uses of the same inum. */ ++xfs->generation; - free(xino); + xfs_free_inode(xino); } } } @@ -844,8 +866,15 @@ xfs_move_entry(struct xfs_fs *xfs, fuse_ino_t inum, XFS_INODE_ALL *xino; XFS_INODE_ALL *parent; XFS_INODE *dest; + char *cpyname; - if (xfs_check_move_entry(xfs, inum, new_parent_inum, name)) + /* Copy the new name. We'll either end up freeing it, or we'll + * use it to replace something else (which gets freed instead) */ + if ((cpyname = strdup(name)) == NULL) + { + result = ENOMEM; + } + else if (xfs_check_move_entry(xfs, inum, new_parent_inum, name)) { xino = xfs->inode_table[inum]; parent = xfs->inode_table[new_parent_inum]; @@ -862,19 +891,31 @@ xfs_move_entry(struct xfs_fs *xfs, fuse_ino_t inum, unlink_inode_from_parent(xino); link_inode_into_directory_node(parent, xino); - strcpy(xino->pub.name, name); + + /* Swap the copy name and the inode name so we end up with the + * right name, and the old one gets freed */ + char *t = xino->pub.name; + xino->pub.name = cpyname; + cpyname = t; } else if (strcmp(xino->pub.name, name) != 0) { /* Same directory, but name has changed */ if ((dest = xfs_lookup_in_dir(xfs, new_parent_inum, name)) != NULL) { + /* Name collision - remove destination entry */ xfs_remove_entry(xfs, dest->inum); } - strcpy(xino->pub.name, name); + + /* Swap the copy name and the inode name so we end up with the + * right name, and the old one gets freed */ + char *t = xino->pub.name; + xino->pub.name = cpyname; + cpyname = t; } result = 0; } + free (cpyname); return result; } diff --git a/sesman/chansrv/chansrv_xfs.h b/sesman/chansrv/chansrv_xfs.h index e180892f..4b82fd20 100644 --- a/sesman/chansrv/chansrv_xfs.h +++ b/sesman/chansrv/chansrv_xfs.h @@ -29,7 +29,11 @@ #include "arch.h" -#define XFS_MAXFILENAMELEN 255 +/* Maximum length of filename supported (in bytes). + * This is a sensible limit to a filename length. It is not used by + * this module to allocate long-lived storage, so it can be increased + * if necessary */ +#define XFS_MAXFILENAMELEN 1023 /* * Incomplete types for the public interface @@ -37,6 +41,9 @@ struct xfs_fs; struct xfs_dir_handle; +/** + * Describe an inode in the XFS filesystem + */ typedef struct xfs_inode { fuse_ino_t inum; /* File serial number. */ @@ -47,7 +54,7 @@ typedef struct xfs_inode time_t atime; /* Time of last access. */ time_t mtime; /* Time of last modification. */ time_t ctime; /* Time of last status change. */ - char name[XFS_MAXFILENAMELEN + 1]; /* Short name */ + char *name; /* Short name (dynamically allocated) */ tui32 generation; /* Changes if inode is reused */ char is_redirected; /* file is on redirected device */ tui32 device_id; /* device ID of redirected device */ From 6c9d56efc2168ec01b977e512c4df482fb0fa649 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 24 Jul 2024 13:27:46 +0100 Subject: [PATCH 2/5] Remove hard-coded filename limit for clipboard file lists The limit of 256 characters for clipboard files is limiting for many Asian locales, particularly as '%xx' notation is used to communicate bytes with bit 7 set. (cherry picked from commit a90228241dffb29c947a168feb82c7407dfa52e2) --- sesman/chansrv/clipboard_file.c | 199 ++++++++++++++++++++------------ 1 file changed, 122 insertions(+), 77 deletions(-) diff --git a/sesman/chansrv/clipboard_file.c b/sesman/chansrv/clipboard_file.c index 5b428017..d6e02c58 100644 --- a/sesman/chansrv/clipboard_file.c +++ b/sesman/chansrv/clipboard_file.c @@ -26,6 +26,7 @@ #include #endif +#include #include #include #include @@ -52,8 +53,8 @@ extern char g_fuse_clipboard_path[]; struct cb_file_info { - char pathname[256]; - char filename[256]; + char *pathname; + char *filename; int flags; int size; tui64 time; @@ -82,6 +83,103 @@ timeval2wintime(struct timeval *tv) } #endif +/** + * Gets a useable filename from a file specification passed to us + * + * The passed-in specification may contain instances of RFC3986 encoded + * octets '%xx' where 'x' is a hex digit (e.g. %20 == ASCII SPACE). For + * UTF-8, there may be many of these (e.g. %E6%97%A5 maps to the U+65E5 + * Unicode character) + * + * The result must be free'd by the caller. + */ +static char * +decode_rfc3986(const char *rfc3986, int len) +{ + char *result = (char *)malloc(len + 1); + if (result != NULL) + { + int i = 0; + int j = 0; + /* Copy the passed-in filename so we can modify it */ + while (i < len) + { + /* Check for %xx for a character (e.g. %20 == ASCII 32 == SPACE) */ + if (rfc3986[i] == '%' && (len - i) > 2 && + isxdigit(rfc3986[i + 1]) && isxdigit(rfc3986[i + 2])) + { + char jchr[] = { rfc3986[i + 1], rfc3986[i + 2], '\0' }; + result[j++] = g_htoi(jchr); + i += 3; + } + else + { + result[j++] = rfc3986[i++]; + } + } + result[j] = '\0'; + } + + return result; +} + +/** + * Allocates a alloc_cb_file_info struct + * + * The memory for the struct is allocated in such a way that a single + * free() call can be used to de-allocate it + * + * Filename elements are copied into the struct + */ +static struct cb_file_info * +alloc_cb_file_info(const char *full_name) +{ + struct cb_file_info *result = NULL; + + /* Find the last path separator in the string */ + const char *psep = strrchr(full_name, '/'); + + /* Separate the name into a path and an unqualified name */ + const char *path_ptr = "/"; + unsigned int path_len = 1; + const char *name_ptr; + + if (psep == NULL) + { + name_ptr = full_name; + } + else if (psep == full_name) + { + name_ptr = full_name + 1; + } + else + { + path_ptr = full_name; + path_len = psep - full_name; + name_ptr = psep + 1; + } + + /* Allocate a block big enough for the struct, and + * for both the strings */ + unsigned int name_len = strlen(name_ptr); + unsigned int alloc_size = sizeof(struct cb_file_info) + + (path_len + 1) + (name_len + 1); + + result = (struct cb_file_info *)malloc(alloc_size); + if (result != NULL) + { + /* Get a pointer to the first byte past the struct */ + result->pathname = (char *)(result + 1); + result->filename = result->pathname + path_len + 1; + memcpy(result->pathname, path_ptr, path_len); + result->pathname[path_len] = '\0'; + memcpy(result->filename, name_ptr, name_len); + result->filename[name_len] = '\0'; + } + + return result; +} + /*** * See MS-RDPECLIP 3.1.5.4.7 * @@ -113,54 +211,13 @@ clipboard_send_filecontents_response_fail(int streamId) return rv; } -/*****************************************************************************/ -/* this will replace %20 or any hex with the space or correct char - * returns error */ -static int -clipboard_check_file(char *filename) -{ - char lfilename[256]; - char jchr[8]; - int lindex; - int index; - - g_memset(lfilename, 0, 256); - lindex = 0; - index = 0; - while (filename[index] != 0) - { - if (filename[index] == '%') - { - jchr[0] = filename[index + 1]; - jchr[1] = filename[index + 2]; - jchr[2] = 0; - index += 3; - lfilename[lindex] = g_htoi(jchr); - lindex++; - } - else - { - lfilename[lindex] = filename[index]; - lindex++; - index++; - } - } - LOG_DEVEL(LOG_LEVEL_DEBUG, "[%s] [%s]", filename, lfilename); - g_strcpy(filename, lfilename); - return 0; -} - /*****************************************************************************/ static int clipboard_get_file(const char *file, int bytes) { - int sindex; - int pindex; - int flags; - char full_fn[256]; /* /etc/xrdp/xrdp.ini */ - char filename[256]; /* xrdp.ini */ - char pathname[256]; /* /etc/xrdp */ + char *full_fn; struct cb_file_info *cfi; + int result = 1; /* x-special/gnome-copied-files */ if ((g_strncmp(file, "copy", 4) == 0) && (bytes == 4)) @@ -171,35 +228,23 @@ clipboard_get_file(const char *file, int bytes) { return 0; } - sindex = 0; - flags = CB_FILE_ATTRIBUTE_ARCHIVE; + /* text/uri-list */ /* x-special/gnome-copied-files */ - if (g_strncmp(file, "file://", 7) == 0) + if (bytes > 7 && g_strncmp(file, "file://", 7) == 0) { - sindex = 7; + full_fn = decode_rfc3986(file + 7, bytes - 7); } - pindex = bytes; - while (pindex > sindex) + else { - if (file[pindex] == '/') - { - break; - } - pindex--; + full_fn = decode_rfc3986(file, bytes); } - g_memset(pathname, 0, 256); - g_memset(filename, 0, 256); - g_memcpy(pathname, file + sindex, pindex - sindex); - if (pathname[0] == 0) + + if (full_fn == NULL) { - pathname[0] = '/'; + LOG(LOG_LEVEL_ERROR, "clipboard_get_file: Out of memory"); + return 1; } - g_memcpy(filename, file + pindex + 1, (bytes - 1) - pindex); - /* this should replace %20 with space */ - clipboard_check_file(pathname); - clipboard_check_file(filename); - g_snprintf(full_fn, 255, "%s/%s", pathname, filename); /* * Before we look at the file, see if it's in the FUSE filesystem. If it is, @@ -209,34 +254,34 @@ clipboard_get_file(const char *file, int bytes) { LOG(LOG_LEVEL_ERROR, "clipboard_get_file: Can't add client-side file " "%s to clipboard", full_fn); - return 1; } - if (g_directory_exist(full_fn)) + else if (g_directory_exist(full_fn)) { LOG(LOG_LEVEL_ERROR, "clipboard_get_file: file [%s] is a directory, " "not supported", full_fn); - flags |= CB_FILE_ATTRIBUTE_DIRECTORY; - return 1; } - if (!g_file_exist(full_fn)) + else if (!g_file_exist(full_fn)) { LOG(LOG_LEVEL_ERROR, "clipboard_get_file: file [%s] does not exist", full_fn); - return 1; + } + else if ((cfi = alloc_cb_file_info(full_fn)) == NULL) + { + LOG(LOG_LEVEL_ERROR, "clipboard_get_file: Out of memory"); } else { - cfi = (struct cb_file_info *)g_malloc(sizeof(struct cb_file_info), 1); list_add_item(g_files_list, (tintptr)cfi); - g_strcpy(cfi->filename, filename); - g_strcpy(cfi->pathname, pathname); cfi->size = g_file_get_size(full_fn); - cfi->flags = flags; + cfi->flags = CB_FILE_ATTRIBUTE_ARCHIVE; cfi->time = (g_time1() + CB_EPOCH_DIFF) * 10000000LL; LOG_DEVEL(LOG_LEVEL_DEBUG, "ok filename [%s] pathname [%s] size [%d]", cfi->filename, cfi->pathname, cfi->size); + result = 0; } - return 0; + + free(full_fn); + return result; } /*****************************************************************************/ From f3070aef158f891ae7deb82911475e8d8a85ac19 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Tue, 30 Jul 2024 13:35:37 +0100 Subject: [PATCH 3/5] Allow for longer filenames from the redirector. This commit ensures that filenames up to the maximum size supported by our xfs can be supported. (cherry picked from commit c3f7eec4f538332b2c2aea447216e7967c361b5d) --- sesman/chansrv/devredir.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sesman/chansrv/devredir.c b/sesman/chansrv/devredir.c index 1a1df6da..e33e2f35 100644 --- a/sesman/chansrv/devredir.c +++ b/sesman/chansrv/devredir.c @@ -53,6 +53,7 @@ #include "log.h" #include "chansrv.h" #include "chansrv_fuse.h" +#include "chansrv_xfs.h" #include "devredir.h" #include "smartcard.h" #include "ms-rdpefs.h" @@ -1249,7 +1250,13 @@ devredir_proc_query_dir_response(IRP *irp, return -1; } + // Size the filename buffer so it's big enough for + // storing the file in our filesystem if we need to. +#ifdef XFS_MAXFILENAMELEN + char filename[XFS_MAXFILENAMELEN + 1]; +#else char filename[256]; +#endif tui64 LastAccessTime; tui64 LastWriteTime; tui64 EndOfFile; From e59dc16be6a30ccd71705a6a5f6344c054486dce Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 1 Aug 2024 12:24:07 +0100 Subject: [PATCH 4/5] Remove unnecessary copy from clipboard_get_files() The routine clipboard_get_files() parses a potentially long string, and copies portions of it into a temporary buffer. This buffer is then passed to clipboard_get_file() as pointer + length; The buffer is inadequately sized for very long filenames which may approach XFS_MAXFILENAMELEN in length. This can cause chansrv to fail when the user copies such filenames. It turns out the buffer is unnecessary, as the filenames can be passed directly into clipboard_get_file() from the source string, using pointer + length. This avoids the length limitation entirely. (cherry picked from commit 34b558246079ef588852c0bd050cee8ae857a338) --- sesman/chansrv/clipboard_file.c | 41 +++++++++++++++++---------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/sesman/chansrv/clipboard_file.c b/sesman/chansrv/clipboard_file.c index d6e02c58..2d515a77 100644 --- a/sesman/chansrv/clipboard_file.c +++ b/sesman/chansrv/clipboard_file.c @@ -285,37 +285,38 @@ clipboard_get_file(const char *file, int bytes) } /*****************************************************************************/ +/* + * Calls clipboard_get_file() for each filename in a list. + * + * List items are separated by line terminators. Blank items are ignored */ static int clipboard_get_files(const char *files, int bytes) { - int index; - int file_index; - char file[512]; + const char *start = files; + const char *end = files + bytes; + const char *p; - file_index = 0; - for (index = 0; index < bytes; index++) + for (p = start ; p < end ; ++p) { - if (files[index] == '\n' || files[index] == '\r') + if (*p == '\n' || *p == '\r') { - if (file_index > 0) + /* Skip zero-length files (which might be caused by + * multiple line terminators */ + if (p > start) { - if (clipboard_get_file(file, file_index) == 0) - { - } - file_index = 0; + /* Get file. Errors are logged */ + (void)clipboard_get_file(start, p - start); } - } - else - { - file[file_index] = files[index]; - file_index++; + + /* Move the start of filename pointer to either 'end', or + * the next character which will either be a filename or + * another terminator */ + start = p + 1; } } - if (file_index > 0) + if (end > start) { - if (clipboard_get_file(file, file_index) == 0) - { - } + (void)clipboard_get_file(start, end - start); } if (g_files_list->count < 1) { From ace386d072fb8ff4851c4798d879241ed4d42def Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 2 Aug 2024 11:57:50 +0100 Subject: [PATCH 5/5] clipboard: Allow a file read to return 0 for EOF When used with a FreeRDP client on Linux, a file copy operation from the clipboard detects end-of-file by a read returning 0 bytes. This is currently marked as an error. It is assumed that mstsc.exe detects end-of-file in another way, which is why this has not been found before. (cherry picked from commit 0f6e731524262cba375d03db312858a2d2bcebdc) --- sesman/chansrv/clipboard_file.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sesman/chansrv/clipboard_file.c b/sesman/chansrv/clipboard_file.c index 2d515a77..90fed048 100644 --- a/sesman/chansrv/clipboard_file.c +++ b/sesman/chansrv/clipboard_file.c @@ -538,10 +538,12 @@ clipboard_send_file_data(int streamId, int lindex, make_stream(s); init_stream(s, cbRequested + 64); size = g_file_read(fd, s->data + 12, cbRequested); - if (size < 1) + // If we're at end-of-file, 0 is a valid response + if (size < 0) { - LOG_DEVEL(LOG_LEVEL_ERROR, "clipboard_send_file_data: read error, want %d got %d", - cbRequested, size); + LOG_DEVEL(LOG_LEVEL_ERROR, + "clipboard_send_file_data: read error, want %d got [%s]", + cbRequested, g_get_strerror()); free_stream(s); g_file_close(fd); clipboard_send_filecontents_response_fail(streamId);