Possible issue with check_dir_not_empty

[Bernhard Übelacker]

Hello Corinna,

Am 18.11.24 um 17:52 schrieb 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!

Forgot to mention the background. I actually hit this issue with running
Cygwin's git.exe below a modified Wine checking out the tag 3.5.3 of
newlib-cygwin. Unfortunately reproducing this issue still needs a few 
additional Wine patches to finish Cygwin installation.


>> 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.


That would be great. Thanks for maintaining Cygwin.

Kind regards,
Bernhard


> 
> 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;