DMARC-Filter: OpenDMARC Filter v1.4.2 delorie.com 4AIGsofK2729106
Authentication-Results: delorie.com; dmarc=pass (p=none dis=none) header.from=cygwin.com
Authentication-Results: delorie.com; spf=pass smtp.mailfrom=cygwin.com
DKIM-Filter: OpenDKIM Filter v2.11.0 delorie.com 4AIGsofK2729106
Authentication-Results: delorie.com;
	dkim=pass (1024-bit key, unprotected) header.d=cygwin.com header.i=@cygwin.com header.a=rsa-sha256 header.s=default header.b=Jk9nRvNx
X-Recipient: archive-cygwin AT delorie DOT com
DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9BE863858CD9
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cygwin.com;
	s=default; t=1731948889;
	bh=whPTHCc3dgOuQxhc3wuAP/KV+F+4mWzLm7ytLydBnAs=;
	h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe:
	 List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:
	 From;
	b=Jk9nRvNxN/wPv85LGt4lNNuqbo38FcJ/Ts8wqpx1CNFZgGv2KZh2Gby5te8zasHRM
	 uy53CcXYypeszmuP/m2NEvvUGrKUmK5NTZr2RZeAIO8Ia5DHXukgKSvk2Q+feTX6vz
	 uFPdZl92NDRDVw1L+oXcTmoBI+l5SlLZ3w9SR7T0=
X-Original-To: cygwin AT cygwin DOT com
Delivered-To: cygwin AT cygwin DOT com
DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C9BA53858C2B
Date: Mon, 18 Nov 2024 17:52:11 +0100
To: Bernhard =?utf-8?Q?=C3=9Cbelacker?= <bernhardu AT mailbox DOT org>
Subject: Re: Possible issue with check_dir_not_empty
Message-ID: <Zztwu6p77XC19HwJ@calimero.vinschen.de>
Mail-Followup-To: Bernhard =?utf-8?Q?=C3=9Cbelacker?= <bernhardu AT mailbox DOT org>, 
 cygwin AT cygwin DOT com
References: <9f95d44b-2a46-4da8-9177-fc9b60a6d18e AT mailbox DOT org>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <9f95d44b-2a46-4da8-9177-fc9b60a6d18e@mailbox.org>
X-BeenThere: cygwin AT cygwin DOT com
X-Mailman-Version: 2.1.30
Precedence: list
List-Id: General Cygwin discussions and problem reports <cygwin.cygwin.com>
List-Unsubscribe: <https://cygwin.com/mailman/options/cygwin>,
 <mailto:cygwin-request AT cygwin DOT com?subject=unsubscribe>
List-Archive: <https://cygwin.com/pipermail/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-request AT cygwin DOT com?subject=help>
List-Subscribe: <https://cygwin.com/mailman/listinfo/cygwin>,
 <mailto:cygwin-request AT cygwin DOT com?subject=subscribe>
From: Corinna Vinschen via Cygwin <cygwin AT cygwin DOT com>
Reply-To: cygwin AT cygwin DOT com
Cc: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>, cygwin AT cygwin DOT com
Content-Type: text/plain; charset="utf-8"
Errors-To: cygwin-bounces~archive-cygwin=delorie DOT com AT cygwin DOT com
Sender: "Cygwin" <cygwin-bounces~archive-cygwin=delorie DOT com AT cygwin DOT com>
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by delorie.com id 4AIGsofK2729106

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 AT mailbox DOT 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 AT mailbox DOT 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


-- 
Problem reports:      https://cygwin.com/problems.html
FAQ:                  https://cygwin.com/faq/
Documentation:        https://cygwin.com/docs.html
Unsubscribe info:     https://cygwin.com/ml/#unsubscribe-simple