DMARC-Filter: OpenDMARC Filter v1.4.2 delorie.com 4AILXK4W2824707
DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8AFEE3858C33
Message-ID: <309f90e4-d7e9-495c-955b-ee264ad74159@mailbox.org>
Date: Mon, 18 Nov 2024 22:32:46 +0100
MIME-Version: 1.0
Subject: Re: Possible issue with check_dir_not_empty
To: cygwin AT cygwin DOT com
References: <9f95d44b-2a46-4da8-9177-fc9b60a6d18e AT mailbox DOT org>
 <Zztwu6p77XC19HwJ AT calimero DOT vinschen DOT de>
Content-Language: de-DE
Autocrypt: addr=bernhardu AT mailbox DOT org; keydata=
 xsBNBFLFOk0BCADSnIOyw5hwhDXc5yKipb7asfzYmBwgRL9NfM9W8fNz8G5iphXTX2cAvrMy
 7CAOaktgz3yvWiJvf3l5GAB7FrA7HaY0sLRbhCcS06jvEAH3HScfaK+qPeJA1mHo11VudH5E
 oaOUKNz6xy5Gb30SuISwVF+i30GeJNi3DTk8zGjqPQHEQiEo8CJ0Hc1r/fUgo+F9f1ugpZNG
 Hn/ZJowpfCGRcNrrRC199MDI2m4q00i1bda8Z1P2PZTUZDWjD3dpzfkvVLSU+68Y49Kyv934
 Qkj6LV/qw+4wnexWtgBCBTklz0ocVAN2wh/6VQ6ny3I3pJd9Phyd5qDmURZ08u8BZXEtABEB
 AAHNK0Jlcm5oYXJkIMOcYmVsYWNrZXIgPGJlcm5oYXJkdUBtYWlsYm94Lm9yZz7CwJUEEwEK
 AD8CGyMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAFiEE7HR0CmaRX6NuV4XV6W+iX4OmQeYF
 AmUarOEFCRf5nZMACgkQ6W+iX4OmQeaArAf/W+zJJHAQX60M9+P6RsZ/eIy+FLGCR1nJptSx
 x3LHDp7qQG1ZrMTyKBwyEcg9HGVWimg+KIdQaZaT3xnpbOGDz05GCDpN6gCiuCpEeLAHVnIp
 glIpHVfpVf3QMzvW+1Sy7bqSnxtXex62eY76SR/tmaGUUrLc5SwKu9xRMxzpT6ALkd76B9/L
 xzf2RyB2wgYEZN9r24vejivsdweBuSE/kZr4rNy1KkBIyosL0M0oOWthuZyy6rSyyJLyPiDV
 IfVgJ3zyqjWtznO4CGcSS3Uqq7YB+kp5KYcPRB373rq17vN/I/5mLV9W66pANzYjEyp4g9mX
 V/AFpX+d20zdrdjGYc7ATQRSxTpNAQgAzY7Lt75go8+7R9OrlxJgLvy2Btad9nI97eiQJ2So
 JW9VF8GpxdTHrHQSst1AuptxlxnsNJ60ajRA8GjQD62VrLwNrm/CJFmKVY1lnwQpk5eM7vdo
 f6zIEd1wLuuYr11tezWEjAky0J7X5sdz3ToPZh+HeIrQYEHexHJe+4vOjelW9nW0un+Hi5jk
 hjw2VwGp8lT+tq9fRxeqUL+L9ctW3btsz/Uf+1h/s3OGo/dlZSTFVAozeHYNZyiyu3Mt9WDs
 Ppbjgz7/FSxmWyworAw1yQeaQyBQgg3S3CH4euepWfBoa71EDH+I4Fw3/17d4wPiR8MXjjSD
 rlMwpV2dd6pj6wARAQABwsB8BBgBCgAmAhsMFiEE7HR0CmaRX6NuV4XV6W+iX4OmQeYFAmUa
 rOEFCRf5nZMACgkQ6W+iX4OmQeZtyQf8CEBfXTO5luF9N3ev6wrwY4sJhJnpoHdrK0s0Tdz9
 Nu0KPxXDhl5OVGWJWDeTZ41FyAtMLVmfBtTa4LNd0zFBvGP5JYoavNME34hOGJIih9mLl+Qu
 nNsVh1okNAYfS2VHOQuESGA9fsSWgli1H84pM1kpqL4vGlVSH+OPAK0H51e6/poQHDHFUq6s
 qmPCDAU0pSYmuuOg/V98WoZW3mtuG1MVLhgNznenctuF7oHx63OdjpCGA0Gv2p78tnPOP2ZZ
 VadUH1G35w/XzlNEPerCul4mdCB/gaDvO4S6Fw7ceBvPy0MutKiJ2BdjD0kW02G3sLLmy8jY
 Xun5X2+YmyYMtA==
In-Reply-To: <Zztwu6p77XC19HwJ@calimero.vinschen.de>
Precedence: list
From: =?utf-8?q?Bernhard_=C3=9Cbelacker_via_Cygwin?= <cygwin AT cygwin DOT com>
Reply-To: =?UTF-8?Q?Bernhard_=C3=9Cbelacker?= <bernhardu AT mailbox DOT org>
Content-Type: text/plain; charset="utf-8"; Format="flowed"
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

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


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