Possible issue with check_dir_not_empty

[Corinna Vinschen]

Hi Bernhard,

On Nov 16 23:36, Bernhard Übelacker via Cygwin wrote:
> Hello everyone,
> 
> Is is about the buffer allocated in check_dir_not_empty.
> 
> The pointer pfni gets allocated the buffer at the begin,
> and is used in the NtQueryDirectoryFile call before the loops.
> In the loop the pointer pfni is also used as iterator.
> Therefore it holds no longer the initial buffer at the call
> to NtQueryDirectoryFile in the while conditition at the bottom.

Good catch, thank you!

> Attached is a possible modification to always use the allocated buffer.
> 
> Kind regards,
> Bernhard

Thanks for the patch.

Would you be ok if I apply a simplified version under your authorship?

Rather than add a pfni_it(erator), use pfni as iterator and add a
pfni_buf variable.  This is a much smaller patch, and is more in line
with the usual variable naming in Cygwin.

I also added a release message text and a Fixes: line to the commit
message.

Below is the tweaked patch.  If you're ok with this version, I'll push
it.


Thanks,
Corinna


>From fbd8b9d769135d6410b423eb9d82b49be52523bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bernhard=20=C3=9Cbelacker?= <bernhardu@mailbox.org>
Date: Sat, 16 Nov 2024 18:09:50 +0100
Subject: [PATCH] Cygwin: check_dir_not_empty: Avoid leaving the allocated
 buffer.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The pointer pfni gets allocated the buffer at the begin,
and is used in the NtQueryDirectoryFile call before the loops.
In the loop the pointer pfni is also used as iterator.
Therefore it holds no longer the initial buffer at the call
to NtQueryDirectoryFile in the while conditition at the bottom.

Fixes: 28fa2a72f8106 ("* syscalls.cc (check_dir_not_empty): Check surplus directory entries")
Signed-off-by: Bernhard Übelacker <bernhardu@mailbox.org>
---
 winsup/cygwin/release/3.5.5 |  3 +++
 winsup/cygwin/syscalls.cc   | 10 ++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/release/3.5.5 b/winsup/cygwin/release/3.5.5
index 2ca4572db7ed..3088f8682b6b 100644
--- a/winsup/cygwin/release/3.5.5
+++ b/winsup/cygwin/release/3.5.5
@@ -33,3 +33,6 @@ Fixes:
 
 - Fix type of pthread_sigqueue() first parameter to match Linux.
   Addresses: https://cygwin.com/pipermail/cygwin/2024-September/256439.html
+
+- Fix potential stack corruption in rmdir() in a border case.
+  Addresses: https://cygwin.com/pipermail/cygwin/2024-November/256774.html
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index df7d3a14efd4..433739cda6e0 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -617,9 +617,10 @@ check_dir_not_empty (HANDLE dir, path_conv &pc)
   IO_STATUS_BLOCK io;
   const ULONG bufsiz = 3 * sizeof (FILE_NAMES_INFORMATION)
 		       + 3 * NAME_MAX * sizeof (WCHAR);
-  PFILE_NAMES_INFORMATION pfni = (PFILE_NAMES_INFORMATION)
-				 alloca (bufsiz);
-  NTSTATUS status = NtQueryDirectoryFile (dir, NULL, NULL, 0, &io, pfni,
+  PFILE_NAMES_INFORMATION pfni_buf = (PFILE_NAMES_INFORMATION)
+				     alloca (bufsiz);
+  PFILE_NAMES_INFORMATION pfni;
+  NTSTATUS status = NtQueryDirectoryFile (dir, NULL, NULL, 0, &io, pfni_buf,
 					  bufsiz, FileNamesInformation,
 					  FALSE, NULL, TRUE);
   if (!NT_SUCCESS (status))
@@ -631,6 +632,7 @@ check_dir_not_empty (HANDLE dir, path_conv &pc)
   int cnt = 1;
   do
     {
+      pfni = pfni_buf;
       while (pfni->NextEntryOffset)
 	{
 	  if (++cnt > 2)
@@ -677,7 +679,7 @@ check_dir_not_empty (HANDLE dir, path_conv &pc)
 	  pfni = (PFILE_NAMES_INFORMATION) ((caddr_t) pfni + pfni->NextEntryOffset);
 	}
     }
-  while (NT_SUCCESS (NtQueryDirectoryFile (dir, NULL, NULL, 0, &io, pfni,
+  while (NT_SUCCESS (NtQueryDirectoryFile (dir, NULL, NULL, 0, &io, pfni_buf,
 					   bufsiz, FileNamesInformation,
 					   FALSE, NULL, FALSE)));
   return STATUS_SUCCESS;
-- 
2.47.0