From 2f05de4dbf9c04008175679a7a7b9e0cf422a2fa Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Thu, 19 Aug 2021 16:42:23 +0200 Subject: [PATCH] Cygwin: fix all usages of NtQueryDirectoryObject Due to reports on the Cygwin mailing list[1][2], it was uncovered that a NtOpenDirectoryObject/NtQueryDirectoryObject/NtClose sequence with NtQueryDirectoryObject iterating over the directory entries, one entry per invocation, is not running atomically. If new entries are inserted into the queried directory, other entries may be moved around and then accidentally show up twice while iterating. Change (almost) all NtQueryDirectoryObject invocations so that it gets a really big buffer (64K) and ideally fetches all entries at once. This appears to work atomically. "Almost" all, because fhandler_procsys::readdir can't be easily changed. [1] https://cygwin.com/pipermail/cygwin/2021-July/248998.html [2] https://cygwin.com/pipermail/cygwin/2021-August/249124.html Fixes: e9c8cb31930bd ("(format_proc_partitions): Revamp loop over existing harddisks by scanning the NT native \Device object directory and looking for Harddisk entries.") Fixes: a998dd7055766 ("Implement advisory file locking.") Fixes: 3b7cd74bfdf56 ("(winpids::enum_processes): Fetch Cygwin processes from listing of shared cygwin object dir in the native NT namespace.") Fixes: 0d6f2b0117aa7 ("syscalls.cc (sync_worker): Rewrite using native NT functions.") Signed-off-by: Corinna Vinschen --- winsup/cygwin/fhandler_proc.cc | 279 ++++++++++++++++++--------------- winsup/cygwin/flock.cc | 52 +++--- winsup/cygwin/pinfo.cc | 38 +++-- winsup/cygwin/release/3.3.0 | 5 + winsup/cygwin/syscalls.cc | 34 ++-- 5 files changed, 236 insertions(+), 172 deletions(-) diff --git a/winsup/cygwin/fhandler_proc.cc b/winsup/cygwin/fhandler_proc.cc index 66d19ab82..a3b5b16a9 100644 --- a/winsup/cygwin/fhandler_proc.cc +++ b/winsup/cygwin/fhandler_proc.cc @@ -1690,15 +1690,6 @@ format_proc_partitions (void *, char *&destbuf) IO_STATUS_BLOCK io; NTSTATUS status; HANDLE dirhdl; - tmp_pathbuf tp; - - char *buf = tp.c_get (); - char *bufptr = buf; - char *ioctl_buf = tp.c_get (); - PWCHAR mp_buf = tp.w_get (); - WCHAR fpath[MAX_PATH]; - WCHAR gpath[MAX_PATH]; - DWORD len; /* Open \Device object directory. */ wchar_t wpath[MAX_PATH] = L"\\Device"; @@ -1712,141 +1703,169 @@ format_proc_partitions (void *, char *&destbuf) return 0; } + tmp_pathbuf tp; + char *buf = tp.c_get (); + char *bufptr = buf; + char *ioctl_buf = tp.c_get (); + PWCHAR mp_buf = tp.w_get (); + PDIRECTORY_BASIC_INFORMATION dbi_buf = (PDIRECTORY_BASIC_INFORMATION) + tp.w_get (); + WCHAR fpath[MAX_PATH]; + WCHAR gpath[MAX_PATH]; + DWORD len; + /* Traverse \Device directory ... */ - PDIRECTORY_BASIC_INFORMATION dbi = (PDIRECTORY_BASIC_INFORMATION) - alloca (640); BOOLEAN restart = TRUE; bool got_one = false; + bool last_run = false; ULONG context = 0; - while (NT_SUCCESS (NtQueryDirectoryObject (dirhdl, dbi, 640, TRUE, restart, - &context, NULL))) + while (!last_run) { - HANDLE devhdl; - PARTITION_INFORMATION_EX *pix = NULL; - PARTITION_INFORMATION *pi = NULL; - DWORD bytes_read; - DWORD part_cnt = 0; - unsigned long long size; - - restart = FALSE; - /* ... and check for a "Harddisk[0-9]*" entry. */ - if (dbi->ObjectName.Length < 9 * sizeof (WCHAR) - || wcsncasecmp (dbi->ObjectName.Buffer, L"Harddisk", 8) != 0 - || !iswdigit (dbi->ObjectName.Buffer[8])) - continue; - /* Got it. Now construct the path to the entire disk, which is - "\\Device\\HarddiskX\\Partition0", and open the disk with - minimum permissions. */ - unsigned long drive_num = wcstoul (dbi->ObjectName.Buffer + 8, NULL, 10); - wcscpy (wpath, dbi->ObjectName.Buffer); - PWCHAR wpart = wpath + dbi->ObjectName.Length / sizeof (WCHAR); - wcpcpy (wpart, L"\\Partition0"); - upath.Length = dbi->ObjectName.Length + 22; - upath.MaximumLength = upath.Length + sizeof (WCHAR); - InitializeObjectAttributes (&attr, &upath, OBJ_CASE_INSENSITIVE, - dirhdl, NULL); - status = NtOpenFile (&devhdl, READ_CONTROL, &attr, &io, - FILE_SHARE_VALID_FLAGS, 0); + status = NtQueryDirectoryObject (dirhdl, dbi_buf, 65536, FALSE, restart, + &context, NULL); if (!NT_SUCCESS (status)) { - debug_printf ("NtOpenFile(%S), status %y", &upath, status); + debug_printf ("NtQueryDirectoryObject, status %y", status); __seterrno_from_nt_status (status); - continue; + break; } - if (!got_one) + if (status != STATUS_MORE_ENTRIES) + last_run = true; + restart = FALSE; + for (PDIRECTORY_BASIC_INFORMATION dbi = dbi_buf; + dbi->ObjectName.Length > 0; + dbi++) { - print ("major minor #blocks name win-mounts\n\n"); - got_one = true; - } - /* Fetch partition info for the entire disk to get its size. */ - if (DeviceIoControl (devhdl, IOCTL_DISK_GET_PARTITION_INFO_EX, NULL, 0, - ioctl_buf, NT_MAX_PATH, &bytes_read, NULL)) - { - pix = (PARTITION_INFORMATION_EX *) ioctl_buf; - size = pix->PartitionLength.QuadPart; - } - else if (DeviceIoControl (devhdl, IOCTL_DISK_GET_PARTITION_INFO, NULL, 0, - ioctl_buf, NT_MAX_PATH, &bytes_read, NULL)) - { - pi = (PARTITION_INFORMATION *) ioctl_buf; - size = pi->PartitionLength.QuadPart; - } - else - { - debug_printf ("DeviceIoControl (%S, " - "IOCTL_DISK_GET_PARTITION_INFO{_EX}) %E", &upath); - size = 0; - } - device dev (drive_num, 0); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s\n", - dev.get_major (), dev.get_minor (), - size >> 10, dev.name () + 5); - /* Fetch drive layout info to get size of all partitions on the disk. */ - if (DeviceIoControl (devhdl, IOCTL_DISK_GET_DRIVE_LAYOUT_EX, - NULL, 0, ioctl_buf, NT_MAX_PATH, &bytes_read, NULL)) - { - PDRIVE_LAYOUT_INFORMATION_EX pdlix = (PDRIVE_LAYOUT_INFORMATION_EX) - ioctl_buf; - part_cnt = pdlix->PartitionCount; - pix = pdlix->PartitionEntry; - } - else if (DeviceIoControl (devhdl, IOCTL_DISK_GET_DRIVE_LAYOUT, - NULL, 0, ioctl_buf, NT_MAX_PATH, &bytes_read, NULL)) - { - PDRIVE_LAYOUT_INFORMATION pdli = (PDRIVE_LAYOUT_INFORMATION) ioctl_buf; - part_cnt = pdli->PartitionCount; - pi = pdli->PartitionEntry; - } - else - debug_printf ("DeviceIoControl(%S, " - "IOCTL_DISK_GET_DRIVE_LAYOUT{_EX}): %E", &upath); - /* Loop over partitions. */ - if (pix || pi) - for (DWORD i = 0; i < part_cnt && i < 64; ++i) - { - DWORD part_num; + HANDLE devhdl; + PARTITION_INFORMATION_EX *pix = NULL; + PARTITION_INFORMATION *pi = NULL; + DWORD bytes_read; + DWORD part_cnt = 0; + unsigned long drive_num; + unsigned long long size; - if (pix) - { - size = pix->PartitionLength.QuadPart; - part_num = pix->PartitionNumber; - ++pix; - } - else - { - size = pi->PartitionLength.QuadPart; - part_num = pi->PartitionNumber; - ++pi; - } - /* A partition number of 0 denotes an extended partition or a - filler entry as described in fhandler_dev_floppy::lock_partition. - Just skip. */ - if (part_num == 0) + /* ... and check for a "Harddisk[0-9]*" entry. */ + if (dbi->ObjectName.Length < 9 * sizeof (WCHAR) + || wcsncasecmp (dbi->ObjectName.Buffer, L"Harddisk", 8) != 0 + || !iswdigit (dbi->ObjectName.Buffer[8])) + continue; + /* Got it. Now construct the path to the entire disk, which is + "\\Device\\HarddiskX\\Partition0", and open the disk with + minimum permissions. */ + drive_num = wcstoul (dbi->ObjectName.Buffer + 8, NULL, 10); + wcscpy (wpath, dbi->ObjectName.Buffer); + PWCHAR wpart = wpath + dbi->ObjectName.Length / sizeof (WCHAR); + wcpcpy (wpart, L"\\Partition0"); + upath.Length = dbi->ObjectName.Length + 22; + upath.MaximumLength = upath.Length + sizeof (WCHAR); + InitializeObjectAttributes (&attr, &upath, OBJ_CASE_INSENSITIVE, + dirhdl, NULL); + status = NtOpenFile (&devhdl, READ_CONTROL, &attr, &io, + FILE_SHARE_VALID_FLAGS, 0); + if (!NT_SUCCESS (status)) + { + debug_printf ("NtOpenFile(%S), status %y", &upath, status); + __seterrno_from_nt_status (status); continue; - device dev (drive_num, part_num); - - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s", - dev.get_major (), dev.get_minor (), - size >> 10, dev.name () + 5); - /* Check if the partition is mounted in Windows and, if so, - print the mount point list. */ - __small_swprintf (fpath, - L"\\\\?\\GLOBALROOT\\Device\\%S\\Partition%u\\", - &dbi->ObjectName, part_num); - if (GetVolumeNameForVolumeMountPointW (fpath, gpath, MAX_PATH) - && GetVolumePathNamesForVolumeNameW (gpath, mp_buf, - NT_MAX_PATH, &len)) + } + if (!got_one) + { + print ("major minor #blocks name win-mounts\n\n"); + got_one = true; + } + /* Fetch partition info for the entire disk to get its size. */ + if (DeviceIoControl (devhdl, IOCTL_DISK_GET_PARTITION_INFO_EX, NULL, + 0, ioctl_buf, NT_MAX_PATH, &bytes_read, NULL)) + { + pix = (PARTITION_INFORMATION_EX *) ioctl_buf; + size = pix->PartitionLength.QuadPart; + } + else if (DeviceIoControl (devhdl, IOCTL_DISK_GET_PARTITION_INFO, NULL, + 0, ioctl_buf, NT_MAX_PATH, &bytes_read, + NULL)) + { + pi = (PARTITION_INFORMATION *) ioctl_buf; + size = pi->PartitionLength.QuadPart; + } + else + { + debug_printf ("DeviceIoControl (%S, " + "IOCTL_DISK_GET_PARTITION_INFO{_EX}) %E", &upath); + size = 0; + } + device dev (drive_num, 0); + bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s\n", + dev.get_major (), dev.get_minor (), + size >> 10, dev.name () + 5); + /* Fetch drive layout info to get size of all partitions on disk. */ + if (DeviceIoControl (devhdl, IOCTL_DISK_GET_DRIVE_LAYOUT_EX, NULL, 0, + ioctl_buf, NT_MAX_PATH, &bytes_read, NULL)) + { + PDRIVE_LAYOUT_INFORMATION_EX pdlix = + (PDRIVE_LAYOUT_INFORMATION_EX) ioctl_buf; + part_cnt = pdlix->PartitionCount; + pix = pdlix->PartitionEntry; + } + else if (DeviceIoControl (devhdl, IOCTL_DISK_GET_DRIVE_LAYOUT, NULL, + 0, ioctl_buf, NT_MAX_PATH, &bytes_read, + NULL)) + { + PDRIVE_LAYOUT_INFORMATION pdli = + (PDRIVE_LAYOUT_INFORMATION) ioctl_buf; + part_cnt = pdli->PartitionCount; + pi = pdli->PartitionEntry; + } + else + debug_printf ("DeviceIoControl(%S, " + "IOCTL_DISK_GET_DRIVE_LAYOUT{_EX}): %E", &upath); + /* Loop over partitions. */ + if (pix || pi) + for (DWORD i = 0; i < part_cnt && i < 64; ++i) { - len = strlen (dev.name () + 5); - while (len++ < 6) - *bufptr++ = ' '; - for (PWCHAR p = mp_buf; *p; p = wcschr (p, L'\0') + 1) - bufptr += __small_sprintf (bufptr, " %W", p); - } + DWORD part_num; - *bufptr++ = '\n'; - } - NtClose (devhdl); + if (pix) + { + size = pix->PartitionLength.QuadPart; + part_num = pix->PartitionNumber; + ++pix; + } + else + { + size = pi->PartitionLength.QuadPart; + part_num = pi->PartitionNumber; + ++pi; + } + /* A partition number of 0 denotes an extended partition or a + filler entry as described in + fhandler_dev_floppy::lock_partition. Just skip. */ + if (part_num == 0) + continue; + device dev (drive_num, part_num); + + bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s", + dev.get_major (), dev.get_minor (), + size >> 10, dev.name () + 5); + /* Check if the partition is mounted in Windows and, if so, + print the mount point list. */ + __small_swprintf (fpath, + L"\\\\?\\GLOBALROOT\\Device\\%S\\Partition%u\\", + &dbi->ObjectName, part_num); + if (GetVolumeNameForVolumeMountPointW (fpath, gpath, MAX_PATH) + && GetVolumePathNamesForVolumeNameW (gpath, mp_buf, + NT_MAX_PATH, &len)) + { + len = strlen (dev.name () + 5); + while (len++ < 6) + *bufptr++ = ' '; + for (PWCHAR p = mp_buf; *p; p = wcschr (p, L'\0') + 1) + bufptr += __small_sprintf (bufptr, " %W", p); + } + + *bufptr++ = '\n'; + } + NtClose (devhdl); + } } NtClose (dirhdl); diff --git a/winsup/cygwin/flock.cc b/winsup/cygwin/flock.cc index ecd2293fa..bd7a16d91 100644 --- a/winsup/cygwin/flock.cc +++ b/winsup/cygwin/flock.cc @@ -596,34 +596,46 @@ lockf_t::from_obj_name (inode_t *node, lockf_t **head, const wchar_t *name) lockf_t * inode_t::get_all_locks_list () { - struct fdbi - { - DIRECTORY_BASIC_INFORMATION dbi; - WCHAR buf[2][NAME_MAX + 1]; - } f; + tmp_pathbuf tp; ULONG context; NTSTATUS status; + BOOLEAN restart = TRUE; + bool last_run = false; lockf_t newlock, *lock = i_all_lf; - for (BOOLEAN restart = TRUE; - NT_SUCCESS (status = NtQueryDirectoryObject (i_dir, &f, sizeof f, TRUE, - restart, &context, NULL)); - restart = FALSE) + PDIRECTORY_BASIC_INFORMATION dbi_buf = (PDIRECTORY_BASIC_INFORMATION) + tp.w_get (); + while (!last_run) { - if (f.dbi.ObjectName.Length != LOCK_OBJ_NAME_LEN * sizeof (WCHAR)) - continue; - f.dbi.ObjectName.Buffer[LOCK_OBJ_NAME_LEN] = L'\0'; - if (!newlock.from_obj_name (this, &i_all_lf, f.dbi.ObjectName.Buffer)) - continue; - if (lock - i_all_lf >= MAX_LOCKF_CNT) + status = NtQueryDirectoryObject (i_dir, dbi_buf, 65536, FALSE, restart, + &context, NULL); + if (!NT_SUCCESS (status)) { - system_printf ("Warning, can't handle more than %d locks per file.", - MAX_LOCKF_CNT); + debug_printf ("NtQueryDirectoryObject, status %y", status); break; } - if (lock > i_all_lf) - lock[-1].lf_next = lock; - new (lock++) lockf_t (newlock); + if (status != STATUS_MORE_ENTRIES) + last_run = true; + restart = FALSE; + for (PDIRECTORY_BASIC_INFORMATION dbi = dbi_buf; + dbi->ObjectName.Length > 0; + dbi++) + { + if (dbi->ObjectName.Length != LOCK_OBJ_NAME_LEN * sizeof (WCHAR)) + continue; + dbi->ObjectName.Buffer[LOCK_OBJ_NAME_LEN] = L'\0'; + if (!newlock.from_obj_name (this, &i_all_lf, dbi->ObjectName.Buffer)) + continue; + if (lock - i_all_lf >= MAX_LOCKF_CNT) + { + system_printf ("Warning, can't handle more than %d locks per file.", + MAX_LOCKF_CNT); + break; + } + if (lock > i_all_lf) + lock[-1].lf_next = lock; + new (lock++) lockf_t (newlock); + } } /* If no lock has been found, return NULL. */ if (lock == i_all_lf) diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc index 06a2eeed4..bce743bfc 100644 --- a/winsup/cygwin/pinfo.cc +++ b/winsup/cygwin/pinfo.cc @@ -1526,23 +1526,35 @@ winpids::enum_processes (bool winpid) if (!winpid) { + tmp_pathbuf tp; + NTSTATUS status; HANDLE dir = get_shared_parent_dir (); BOOLEAN restart = TRUE; - ULONG context; - struct fdbi + bool last_run = false; + ULONG context = 0; + PDIRECTORY_BASIC_INFORMATION dbi_buf = (PDIRECTORY_BASIC_INFORMATION) + tp.w_get (); + while (!last_run) { - DIRECTORY_BASIC_INFORMATION dbi; - WCHAR buf[2][NAME_MAX + 1]; - } f; - while (NT_SUCCESS (NtQueryDirectoryObject (dir, &f, sizeof f, TRUE, - restart, &context, NULL))) - { - restart = FALSE; - f.dbi.ObjectName.Buffer[f.dbi.ObjectName.Length / sizeof (WCHAR)] = L'\0'; - if (wcsncmp (f.dbi.ObjectName.Buffer, L"winpid.", 7) == 0) + status = NtQueryDirectoryObject (dir, dbi_buf, 65536, FALSE, restart, + &context, NULL); + if (!NT_SUCCESS (status)) { - DWORD pid = wcstoul (f.dbi.ObjectName.Buffer + 7, NULL, 10); - add (nelem, false, pid); + debug_printf ("NtQueryDirectoryObject, status %y", status); + break; + } + if (status != STATUS_MORE_ENTRIES) + last_run = true; + restart = FALSE; + for (PDIRECTORY_BASIC_INFORMATION dbi = dbi_buf; + dbi->ObjectName.Length > 0; + dbi++) + { + if (wcsncmp (dbi->ObjectName.Buffer, L"winpid.", 7) == 0) + { + DWORD pid = wcstoul (dbi->ObjectName.Buffer + 7, NULL, 10); + add (nelem, false, pid); + } } } } diff --git a/winsup/cygwin/release/3.3.0 b/winsup/cygwin/release/3.3.0 index 6c7369bbe..0494cd5ab 100644 --- a/winsup/cygwin/release/3.3.0 +++ b/winsup/cygwin/release/3.3.0 @@ -62,3 +62,8 @@ Bug Fixes - Fix getaddrinfo(3) to return valid ai_socktype and ai_protocol values if the underlying GetAddrInfoW screwes up. Addresses: https://cygwin.com/pipermail/cygwin/2021-July/248985.html + +- Fix duplicate /proc/partitions entries and (presumably) duplicate PIDs + in ps(1) output. + Addresses: https://cygwin.com/pipermail/cygwin/2021-July/248998.html + https://cygwin.com/pipermail/cygwin/2021-August/249124.html diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index 6ba4f10f7..6f7efa928 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -2050,19 +2050,35 @@ sync () return; } /* Traverse \Device directory ... */ - PDIRECTORY_BASIC_INFORMATION dbi = (PDIRECTORY_BASIC_INFORMATION) - alloca (640); + tmp_pathbuf tp; + PDIRECTORY_BASIC_INFORMATION dbi_buf = (PDIRECTORY_BASIC_INFORMATION) + tp.w_get (); BOOLEAN restart = TRUE; + bool last_run = false; ULONG context = 0; - while (NT_SUCCESS (NtQueryDirectoryObject (devhdl, dbi, 640, TRUE, restart, - &context, NULL))) + while (!last_run) { + status = NtQueryDirectoryObject (devhdl, dbi_buf, 65536, FALSE, restart, + &context, NULL); + if (!NT_SUCCESS (status)) + { + debug_printf ("NtQueryDirectoryObject, status %y", status); + break; + } + if (status != STATUS_MORE_ENTRIES) + last_run = true; restart = FALSE; - /* ... and call sync_worker for each HarddiskVolumeX entry. */ - if (dbi->ObjectName.Length >= 15 * sizeof (WCHAR) - && !wcsncasecmp (dbi->ObjectName.Buffer, L"HarddiskVolume", 14) - && iswdigit (dbi->ObjectName.Buffer[14])) - sync_worker (devhdl, dbi->ObjectName.Length, dbi->ObjectName.Buffer); + for (PDIRECTORY_BASIC_INFORMATION dbi = dbi_buf; + dbi->ObjectName.Length > 0; + dbi++) + { + /* ... and call sync_worker for each HarddiskVolumeX entry. */ + if (dbi->ObjectName.Length >= 15 * sizeof (WCHAR) + && !wcsncasecmp (dbi->ObjectName.Buffer, L"HarddiskVolume", 14) + && iswdigit (dbi->ObjectName.Buffer[14])) + sync_worker (devhdl, dbi->ObjectName.Length, + dbi->ObjectName.Buffer); + } } NtClose (devhdl); }