- From 9f482599ce0a06894f290827aeac687ce1cb8d50 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Sat, 16 Aug 2025 12:00:22 +0200
- Subject: [PATCH 1/5] sys: |nfs41_Create()| should release Fcb if it acquired
- it itself
- |nfs41_Create()| should release Fcb if it acquired it itself.
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- sys/nfs41sys_openclose.c | 15 ++++++++++++++-
- 1 file changed, 14 insertions(+), 1 deletion(-)
- diff --git a/sys/nfs41sys_openclose.c b/sys/nfs41sys_openclose.c
- index fc1821f..1f75cae 100644
- --- a/sys/nfs41sys_openclose.c
- +++ b/sys/nfs41sys_openclose.c
- @@ -605,6 +605,7 @@ NTSTATUS nfs41_Create(
- __notnull PNFS41_FCB nfs41_fcb = (PNFS41_FCB)Fcb->Context;
- PNFS41_FOBX nfs41_fobx = NULL;
- BOOLEAN oldDeletePending = nfs41_fcb->StandardInfo.DeletePending;
- + bool fcb_locked_exclusive = false;
- #ifdef ENABLE_TIMINGS
- LARGE_INTEGER t1, t2;
- t1 = KeQueryPerformanceCounter(NULL);
- @@ -839,7 +840,15 @@ retry_on_link:
- if (!RxIsFcbAcquiredExclusive(Fcb)) {
- ASSERT(!RxIsFcbAcquiredShared(Fcb));
- - RxAcquireExclusiveFcbResourceInMRx(Fcb);
- +
- + DbgP("nfs41_Create: Fcb not locked exclusive\n");
- +
- + NTSTATUS rxa_status;
- + rxa_status = RxAcquireExclusiveFcbResourceInMRx(Fcb);
- + if (!NT_SUCCESS(rxa_status)) {
- + goto out;
- + }
- + fcb_locked_exclusive = true;
- }
- RxContext->pFobx = RxCreateNetFobx(RxContext, SrvOpen);
- @@ -1057,6 +1066,10 @@ retry_on_link:
- status = RxContext->CurrentIrp->IoStatus.Status = STATUS_SUCCESS;
- out:
- + if (fcb_locked_exclusive) {
- + RxReleaseFcbResourceInMRx(Fcb);
- + }
- +
- if (entry) {
- nfs41_UpcallDestroy(entry);
- }
- --
- 2.45.1
- From f274d5c34417c2e82f8adaac1089dfd822d89a43 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Sat, 16 Aug 2025 12:28:59 +0200
- Subject: [PATCH 2/5] sys: Use macros to get the FCB/FOBX extensions
- Use macros to get the FCB/FOBX extensions.
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- sys/nfs41sys_driver.c | 2 +-
- sys/nfs41sys_lock.c | 4 ++--
- sys/nfs41sys_openclose.c | 6 +++---
- 3 files changed, 6 insertions(+), 6 deletions(-)
- diff --git a/sys/nfs41sys_driver.c b/sys/nfs41sys_driver.c
- index 4de1ab9..b079180 100644
- --- a/sys/nfs41sys_driver.c
- +++ b/sys/nfs41sys_driver.c
- @@ -1319,7 +1319,7 @@ VOID fcbopen_main(PVOID ctx)
- psrvEntry = psrvEntry->Flink;
- };
- }
- - nfs41_fcb = (PNFS41_FCB)cur->fcb->Context;
- + nfs41_fcb = NFS41GetFcbExtension(cur->fcb);
- nfs41_fcb->changeattr = entry->ChangeTime;
- out:
- nfs41_UpcallDestroy(entry);
- diff --git a/sys/nfs41sys_lock.c b/sys/nfs41sys_lock.c
- index 7bd2132..6be928c 100644
- --- a/sys/nfs41sys_lock.c
- +++ b/sys/nfs41sys_lock.c
- @@ -273,7 +273,7 @@ NTSTATUS nfs41_Lock(
- NFS41GetNetRootExtension(SrvOpen->pVNetRoot->pNetRoot);
- #ifdef NFS41_DRIVER_HACK_LOCKING_STORAGE32_RANGELOCK_PROBING
- __notnull PMRX_FCB Fcb = RxContext->pFcb;
- - __notnull PNFS41_FCB nfs41_fcb = (PNFS41_FCB)Fcb->Context;
- + __notnull PNFS41_FCB nfs41_fcb = NFS41GetFcbExtension(Fcb);
- #endif /* NFS41_DRIVER_HACK_LOCKING_STORAGE32_RANGELOCK_PROBING */
- const ULONG flags = LowIoContext->ParamsFor.Locks.Flags;
- LARGE_INTEGER poll_delay = {0};
- @@ -398,7 +398,7 @@ NTSTATUS nfs41_Unlock(
- NFS41GetNetRootExtension(SrvOpen->pVNetRoot->pNetRoot);
- #ifdef NFS41_DRIVER_HACK_LOCKING_STORAGE32_RANGELOCK_PROBING
- __notnull PMRX_FCB Fcb = RxContext->pFcb;
- - __notnull PNFS41_FCB nfs41_fcb = (PNFS41_FCB)Fcb->Context;
- + __notnull PNFS41_FCB nfs41_fcb = NFS41GetFcbExtension(Fcb);
- #endif /* NFS41_DRIVER_HACK_LOCKING_STORAGE32_RANGELOCK_PROBING */
- #ifdef ENABLE_TIMINGS
- LARGE_INTEGER t1, t2;
- diff --git a/sys/nfs41sys_openclose.c b/sys/nfs41sys_openclose.c
- index 1f75cae..c269bc1 100644
- --- a/sys/nfs41sys_openclose.c
- +++ b/sys/nfs41sys_openclose.c
- @@ -480,7 +480,7 @@ NTSTATUS check_nfs41_create_args(
- __notnull PNFS41_NETROOT_EXTENSION pNetRootContext =
- NFS41GetNetRootExtension(SrvOpen->pVNetRoot->pNetRoot);
- __notnull PMRX_FCB Fcb = RxContext->pFcb;
- - __notnull PNFS41_FCB nfs41_fcb = (PNFS41_FCB)Fcb->Context;
- + __notnull PNFS41_FCB nfs41_fcb = NFS41GetFcbExtension(Fcb);
- PFILE_FULL_EA_INFORMATION ea = (PFILE_FULL_EA_INFORMATION)
- RxContext->CurrentIrp->AssociatedIrp.SystemBuffer;
- @@ -602,7 +602,7 @@ NTSTATUS nfs41_Create(
- __notnull PNFS41_NETROOT_EXTENSION pNetRootContext =
- NFS41GetNetRootExtension(SrvOpen->pVNetRoot->pNetRoot);
- __notnull PMRX_FCB Fcb = RxContext->pFcb;
- - __notnull PNFS41_FCB nfs41_fcb = (PNFS41_FCB)Fcb->Context;
- + __notnull PNFS41_FCB nfs41_fcb = NFS41GetFcbExtension(Fcb);
- PNFS41_FOBX nfs41_fobx = NULL;
- BOOLEAN oldDeletePending = nfs41_fcb->StandardInfo.DeletePending;
- bool fcb_locked_exclusive = false;
- @@ -859,7 +859,7 @@ retry_on_link:
- #ifdef DEBUG_OPEN
- DbgP("nfs41_Create: created FOBX 0x%p\n", RxContext->pFobx);
- #endif
- - nfs41_fobx = (PNFS41_FOBX)(RxContext->pFobx)->Context;
- + nfs41_fobx = NFS41GetFobxExtension(RxContext->pFobx);
- nfs41_fobx->nfs41_open_state = entry->open_state;
- if (nfs41_fobx->sec_ctx.ClientToken == NULL) {
- status = nfs41_get_sec_ctx(SecurityImpersonation, &nfs41_fobx->sec_ctx);
- --
- 2.45.1
- From 2bfbab701714d2b287505ba255f95117ae96ac15 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Sat, 16 Aug 2025 17:39:27 +0200
- Subject: [PATCH 3/5] daemon,sys,tests: Handle chars outside Unicode BMP
- correctly
- Handle characters outside the Unicode BMP (Basic Multilingual Plane)
- correctly.
- Reported-by: Dan Shelton <dan.f.shelton@gmail.com>
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- daemon/open.c | 21 +++++++++++++++------
- daemon/symlink.c | 18 +++++++++++-------
- sys/nfs41sys_openclose.c | 10 ++++++----
- sys/nfs41sys_symlink.c | 1 -
- tests/manual_testing.txt | 21 ++++++++++++++++++++-
- 5 files changed, 52 insertions(+), 19 deletions(-)
- diff --git a/daemon/open.c b/daemon/open.c
- index ae1540c..1076053 100644
- --- a/daemon/open.c
- +++ b/daemon/open.c
- @@ -1220,26 +1220,35 @@ static int marshall_open(unsigned char *buffer, uint32_t *length, nfs41_upcall *
- if (status) goto out;
- if (upcall->last_error == ERROR_REPARSE) {
- unsigned short len = (args->symlink.len + 1) * sizeof(WCHAR);
- + int wc_len;
- status = safe_write(&buffer, length, &args->symlink_embedded, sizeof(BOOLEAN));
- if (status) goto out;
- BYTE tmp_symlinktarget_type = args->symlinktarget_type;
- status = safe_write(&buffer, length, &tmp_symlinktarget_type, sizeof(BYTE));
- if (status) goto out;
- - status = safe_write(&buffer, length, &len, sizeof(len));
- - if (status) goto out;
- /*
- * convert args->symlink to wchar
- - * FIXME: What about |len| if we have characters outside the BMP ?
- */
- - if (*length <= len || !MultiByteToWideChar(CP_UTF8,
- + unsigned short *wc_len_out = (unsigned short *)buffer;
- + unsigned short dummy;
- + status = safe_write(&buffer, length, &dummy, sizeof(dummy));
- + if (status) goto out;
- +
- + if (*length <= len) {
- + status = ERROR_BUFFER_OVERFLOW;
- + goto out;
- + }
- + wc_len = MultiByteToWideChar(CP_UTF8,
- MB_ERR_INVALID_CHARS,
- args->symlink.path, args->symlink.len,
- - (LPWSTR)buffer, len / sizeof(WCHAR))) {
- + (LPWSTR)buffer, len / sizeof(WCHAR));
- + if (wc_len == 0) {
- status = ERROR_BUFFER_OVERFLOW;
- goto out;
- }
- - *length -= len;
- + *wc_len_out = (unsigned short)(wc_len*sizeof(wchar_t));
- + *length -= *wc_len_out;
- }
- DPRINTF(2, ("NFS41_SYSOP_OPEN: downcall "
- "open_state=0x%p "
- diff --git a/daemon/symlink.c b/daemon/symlink.c
- index 0a53de4..fef7970 100644
- --- a/daemon/symlink.c
- +++ b/daemon/symlink.c
- @@ -287,9 +287,12 @@ static int marshall_symlink_get(unsigned char *buffer, uint32_t *length,
- {
- symlink_upcall_args *args = &upcall->args.symlink;
- unsigned short len = (args->target_get.len + 1) * sizeof(WCHAR);
- + unsigned short dummy = 0;
- int status = NO_ERROR;
- + int wc_len;
- - status = safe_write(&buffer, length, &len, sizeof(len));
- + unsigned short *wc_len_out = (unsigned short *)buffer;
- + status = safe_write(&buffer, length, &dummy, sizeof(dummy));
- if (status) goto out;
- if (*length <= len) {
- @@ -297,11 +300,11 @@ static int marshall_symlink_get(unsigned char *buffer, uint32_t *length,
- goto out;
- }
- - /* FIXME: What about |len| if we have characters outside the BMP ? */
- - if (!MultiByteToWideChar(CP_UTF8,
- - MB_ERR_INVALID_CHARS,
- - args->target_get.path, args->target_get.len,
- - (LPWSTR)buffer, len / sizeof(WCHAR))) {
- + wc_len = MultiByteToWideChar(CP_UTF8,
- + MB_ERR_INVALID_CHARS,
- + args->target_get.path, args->target_get.len,
- + (LPWSTR)buffer, len / sizeof(WCHAR));
- + if (wc_len == 0) {
- eprintf("marshall_symlink_get: "
- "MultiByteToWideChar() failed, lasterr=%d\n",
- (int)GetLastError());
- @@ -309,7 +312,8 @@ static int marshall_symlink_get(unsigned char *buffer, uint32_t *length,
- goto out;
- }
- - *length -= len;
- + *wc_len_out = (unsigned short)(wc_len*sizeof(wchar_t));
- + *length -= *wc_len_out;
- out:
- return status;
- diff --git a/sys/nfs41sys_openclose.c b/sys/nfs41sys_openclose.c
- index c269bc1..f7bd38d 100644
- --- a/sys/nfs41sys_openclose.c
- +++ b/sys/nfs41sys_openclose.c
- @@ -283,11 +283,11 @@ NTSTATUS unmarshal_nfs41_open(
- RtlCopyMemory(&tmp_symlinktarget_type, *buf, sizeof(BYTE));
- cur->u.Open.symlinktarget_type = tmp_symlinktarget_type;
- *buf += sizeof(BYTE);
- - RtlCopyMemory(&cur->u.Open.symlink.MaximumLength, *buf,
- + RtlCopyMemory(&cur->u.Open.symlink.Length, *buf,
- sizeof(USHORT));
- *buf += sizeof(USHORT);
- - cur->u.Open.symlink.Length = cur->u.Open.symlink.MaximumLength -
- - sizeof(WCHAR);
- + cur->u.Open.symlink.MaximumLength =
- + cur->u.Open.symlink.Length+sizeof(wchar_t);
- cur->u.Open.symlink.Buffer = RxAllocatePoolWithTag(NonPagedPoolNx,
- cur->u.Open.symlink.MaximumLength, NFS41_MM_POOLTAG);
- if (cur->u.Open.symlink.Buffer == NULL) {
- @@ -296,7 +296,9 @@ NTSTATUS unmarshal_nfs41_open(
- goto out;
- }
- RtlCopyMemory(cur->u.Open.symlink.Buffer, *buf,
- - cur->u.Open.symlink.MaximumLength);
- + cur->u.Open.symlink.Length);
- + cur->u.Open.symlink.Buffer[cur->u.Open.symlink.Length/sizeof(wchar_t)] =
- + L'\0';
- #ifdef DEBUG_MARSHAL_DETAIL
- DbgP("unmarshal_nfs41_open: ERROR_REPARSE -> '%wZ'\n", &cur->u.Open.symlink);
- #endif
- diff --git a/sys/nfs41sys_symlink.c b/sys/nfs41sys_symlink.c
- index 43334a1..11b470f 100644
- --- a/sys/nfs41sys_symlink.c
- +++ b/sys/nfs41sys_symlink.c
- @@ -135,7 +135,6 @@ void unmarshal_nfs41_symlink(
- }
- RtlCopyMemory(cur->u.Symlink.target->Buffer, *buf,
- cur->u.Symlink.target->Length);
- - cur->u.Symlink.target->Length -= sizeof(UNICODE_NULL);
- }
- NTSTATUS map_symlink_errors(
- diff --git a/tests/manual_testing.txt b/tests/manual_testing.txt
- index 9500021..c9a5fd6 100644
- --- a/tests/manual_testing.txt
- +++ b/tests/manual_testing.txt
- @@ -1,5 +1,5 @@
- #
- -# ms-nfs41-client manual testing sequence, 2025-07-23
- +# ms-nfs41-client manual testing sequence, 2025-08-16
- #
- # Draft version, needs to be turned into automated tests
- # if possible
- @@ -277,6 +277,25 @@ rm -f symlink1_to_h_tmp ; cmd /C 'mklink /D symlink1_to_h_tmp \\derfwpc5131_ipv4
- # ("." is a relative symlink, just included here for more coverage)
- ksh93 -c 'typeset -a a=( "/" "/usr" "/usr/" "/tmp" "/usr/bin" "$PWD" "/dev/null" "." ) ; for ((i=0 ; i < ${#a[@]} ; i++ )) ; do rm -f "syml$i" ; ln -sf "${a[$i]}" "syml$i" ; l="$(readlink "syml$i")" ; if [[ "$l" == "${a[$i]}" ]] ; then printf "OK test %d\n" i ; else printf "FAIL test %d\n" i ; fi ; done'
- +#
- +# Tests for symlinks with characters outside the BMP (Basic Multilingual Plane)
- +# (Unicode "half moon" in this case)
- +#
- +# ToDo:
- +# - Use different characters with different byte lengths
- +# - Add more tests with relative and absolute paths
- +# - Add tests with subdirs with non-BMP characters
- +#
- +1. Original test:
- +---- snip ----
- +bash -c 'm="$(printf "\U0001f313")" ; echo "Unicode moon character in filename test OK" >"$m$m$m$m$m$m${m}x" && ln -sf "$m$m$m$m$m$m${m}x" "$m$m$m$m$m$m$m" && cat "$m$m$m$m$m$m${m}"'
- +Unicode moon character in filename test OK
- +---- snip ----
- +2. Mix of Unicode non-BMP characters and ASCII charatcers:
- +---- snip ----
- +bash -c 'set -o errexit ; m="$(printf "\U0001f313")" ; typeset filenames=( "$m$m$m$m$m$m$m" "$m$m$m$m$m$m$m$m" "x$m$m$m$m$m$m" "$m$m$m_$m$m$m" ) ; for fn in "${filenames[@]}" ; do printf "Unicode moon character in filename %q test OK\n" "$fn" >"${fn}x" && ln -sf "${fn}x" "${fn}" && cat "$fn" ; done' && ls -l && echo "## All tests OK"
- +---- snip ----
- +
- #
- # Test whether cmd.exe can follow symlink to "/" (root), which should point to C:cygwin64\ or C:\cygwin\
- # (CYGWIN='winsymlinks:nativestrict' is used so the same test works with NTFS too)
- --
- 2.45.1
- From 26d93a2686831fc1ee8a12a751dc19a5ffc7df71 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Sat, 16 Aug 2025 18:04:08 +0200
- Subject: [PATCH 4/5] daemon: Add new helper function
- |get_safe_write_bufferpos()|
- Add new helper function |get_safe_write_bufferpos()|.
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- daemon/open.c | 6 +++---
- daemon/symlink.c | 7 +++----
- daemon/util.c | 17 +++++++++++++++++
- daemon/util.h | 2 ++
- 4 files changed, 25 insertions(+), 7 deletions(-)
- diff --git a/daemon/open.c b/daemon/open.c
- index 1076053..dd10c38 100644
- --- a/daemon/open.c
- +++ b/daemon/open.c
- @@ -1229,9 +1229,9 @@ static int marshall_open(unsigned char *buffer, uint32_t *length, nfs41_upcall *
- /*
- * convert args->symlink to wchar
- */
- - unsigned short *wc_len_out = (unsigned short *)buffer;
- - unsigned short dummy;
- - status = safe_write(&buffer, length, &dummy, sizeof(dummy));
- + unsigned short *wc_len_out;
- + status = get_safe_write_bufferpos(&buffer, length,
- + sizeof(unsigned short), &wc_len_out);
- if (status) goto out;
- if (*length <= len) {
- diff --git a/daemon/symlink.c b/daemon/symlink.c
- index fef7970..dc464f6 100644
- --- a/daemon/symlink.c
- +++ b/daemon/symlink.c
- @@ -287,13 +287,12 @@ static int marshall_symlink_get(unsigned char *buffer, uint32_t *length,
- {
- symlink_upcall_args *args = &upcall->args.symlink;
- unsigned short len = (args->target_get.len + 1) * sizeof(WCHAR);
- - unsigned short dummy = 0;
- int status = NO_ERROR;
- int wc_len;
- - unsigned short *wc_len_out = (unsigned short *)buffer;
- - status = safe_write(&buffer, length, &dummy, sizeof(dummy));
- - if (status) goto out;
- + unsigned short *wc_len_out;
- + status = get_safe_write_bufferpos(&buffer, length,
- + sizeof(unsigned short), &wc_len_out);
- if (*length <= len) {
- status = ERROR_BUFFER_OVERFLOW;
- diff --git a/daemon/util.c b/daemon/util.c
- index aac239d..f5d67ec 100644
- --- a/daemon/util.c
- +++ b/daemon/util.c
- @@ -61,6 +61,23 @@ int safe_write(unsigned char **pos, uint32_t *remaining, void *src, uint32_t src
- return 0;
- }
- +/*
- + * |get_safe_write_bufferpos()| - like |safe_write()| but tests whether we
- + * have enough buffer space left, and in that case return current buffer
- + * position in |destbuffer|
- + */
- +int get_safe_write_bufferpos(unsigned char **pos, uint32_t *remaining, uint32_t src_len, void **destbuffer)
- +{
- + if (*remaining < src_len)
- + return ERROR_BUFFER_OVERFLOW;
- +
- + *destbuffer = *pos;
- + *pos += src_len;
- + *remaining -= src_len;
- + return ERROR_SUCCESS;
- +}
- +
- +
- int get_name(unsigned char **pos, uint32_t *remaining, const char **out_name)
- {
- int status;
- diff --git a/daemon/util.h b/daemon/util.h
- index 53c0ad3..8dba1a9 100644
- --- a/daemon/util.h
- +++ b/daemon/util.h
- @@ -71,6 +71,8 @@ void *mempcpy(void *restrict dest, const void *restrict src, size_t n)
- int safe_read(unsigned char **pos, uint32_t *remaining, void *dest, uint32_t dest_len);
- int safe_write(unsigned char **pos, uint32_t *remaining, void *dest, uint32_t dest_len);
- +int get_safe_write_bufferpos(unsigned char **pos, uint32_t *remaining,
- + uint32_t src_len, void **destbuffer);
- int get_name(unsigned char **pos, uint32_t *remaining, const char **out_name);
- const char* strip_path(
- --
- 2.45.1
- From 91340b796dd64304ed5a6fb1e4b8d20a9bb47394 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Sat, 16 Aug 2025 18:10:50 +0200
- Subject: [PATCH 5/5] daemon: Remove empty |marshall_symlink_set()| function
- Remove empty |marshall_symlink_set()| function.
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- daemon/symlink.c | 7 -------
- 1 file changed, 7 deletions(-)
- diff --git a/daemon/symlink.c b/daemon/symlink.c
- index dc464f6..09cb07d 100644
- --- a/daemon/symlink.c
- +++ b/daemon/symlink.c
- @@ -409,15 +409,8 @@ out:
- return status;
- }
- -static int marshall_symlink_set(unsigned char *buffer, uint32_t *length,
- - nfs41_upcall *upcall)
- -{
- - return NO_ERROR;
- -}
- -
- const nfs41_upcall_op nfs41_op_symlink_set = {
- .parse = parse_symlink_set,
- .handle = handle_symlink_set,
- - .marshall = marshall_symlink_set,
- .arg_size = sizeof(symlink_upcall_args)
- };
- --
- 2.45.1
msnfs41client: Patches for filenames with Unicode non-BMP characters, missing FCB unlock, cleanup+misc, 2025-08-16
Posted by Anonymous on Sat 16th Aug 2025 17:19
raw | new post
Submit a correction or amendment below (click here to make a fresh posting)
After submitting an amendment, you'll be able to view the differences between the old and new posts easily.