From cb41c375a6e2c70e5146c1af68549c1a367320da Mon Sep 17 00:00:00 2001 From: Ben Wijen Date: Fri, 22 Jan 2021 16:47:11 +0100 Subject: [PATCH] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE I think we don't need an extra flag as we can utilize: access & FILE_WRITE_ATTRIBUTES What do you think? Ben Wijen (1): syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE winsup/cygwin/ntdll.h | 3 ++- winsup/cygwin/syscalls.cc | 22 +++++++-------- winsup/cygwin/wincap.cc | 11 ++++++++ winsup/cygwin/wincap.h | 56 ++++++++++++++++++++------------------- 4 files changed, 53 insertions(+), 39 deletions(-) -- 2.30.0 >From 2d0ff6fec10d03c24d11c747852018b7bc1136ac Mon Sep 17 00:00:00 2001 In-Reply-To: <20210122105201.GD810271@calimero.vinschen.de> References: <20210122105201.GD810271@calimero.vinschen.de> From: Ben Wijen Date: Tue, 17 Dec 2019 15:15:25 +0100 Subject: [PATCH v3 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE Implement wincap.has_posix_unlink_semantics_with_ignore_readonly and when set skip setting/clearing of READONLY attribute and instead use FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE --- winsup/cygwin/ntdll.h | 3 ++- winsup/cygwin/syscalls.cc | 22 +++++++-------- winsup/cygwin/wincap.cc | 11 ++++++++ winsup/cygwin/wincap.h | 56 ++++++++++++++++++++------------------- 4 files changed, 53 insertions(+), 39 deletions(-) diff --git a/winsup/cygwin/ntdll.h b/winsup/cygwin/ntdll.h index d4f6aaf45..7eee383dd 100644 --- a/winsup/cygwin/ntdll.h +++ b/winsup/cygwin/ntdll.h @@ -497,7 +497,8 @@ enum { FILE_DISPOSITION_DELETE = 0x01, FILE_DISPOSITION_POSIX_SEMANTICS = 0x02, FILE_DISPOSITION_FORCE_IMAGE_SECTION_CHECK = 0x04, - FILE_DISPOSITION_ON_CLOSE = 0x08 + FILE_DISPOSITION_ON_CLOSE = 0x08, + FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE = 0x10, }; enum diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index c985142eb..b3a110aaa 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -719,7 +719,7 @@ _unlink_nt (path_conv &pc, bool shareable) pc.get_object_attr (attr, sec_none_nih); - /* First check if we can use POSIX unlink semantics: W10 1709++, local NTFS. + /* First check if we can use POSIX unlink semantics: W10 1709+, local NTFS. With POSIX unlink semantics the entire job gets MUCH easier and faster. Just try to do it and if it fails, it fails. */ if (wincap.has_posix_unlink_semantics () @@ -727,20 +727,18 @@ _unlink_nt (path_conv &pc, bool shareable) { FILE_DISPOSITION_INFORMATION_EX fdie; - if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY) + /* POSIX unlink semantics are nice, but they still fail if the file has + the R/O attribute set. If so, ignoring might be an option: W10 1809+ + Removing the file is very much a safe bet afterwards, so, no + transaction. */ + if ((pc.file_attributes () & FILE_ATTRIBUTE_READONLY) + && !wincap.has_posix_unlink_semantics_with_ignore_readonly ()) access |= FILE_WRITE_ATTRIBUTES; status = NtOpenFile (&fh, access, &attr, &io, FILE_SHARE_VALID_FLAGS, flags); if (!NT_SUCCESS (status)) goto out; - /* Why didn't the devs add a FILE_DELETE_IGNORE_READONLY_ATTRIBUTE - flag just like they did with FILE_LINK_IGNORE_READONLY_ATTRIBUTE - and FILE_LINK_IGNORE_READONLY_ATTRIBUTE??? - - POSIX unlink semantics are nice, but they still fail if the file - has the R/O attribute set. Removing the file is very much a safe - bet afterwards, so, no transaction. */ - if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY) + if (access & FILE_WRITE_ATTRIBUTES) { status = NtSetAttributesFile (fh, pc.file_attributes () & ~FILE_ATTRIBUTE_READONLY); @@ -751,10 +749,12 @@ _unlink_nt (path_conv &pc, bool shareable) } } fdie.Flags = FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS; + if (wincap.has_posix_unlink_semantics_with_ignore_readonly ()) + fdie.Flags |= FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE; status = NtSetInformationFile (fh, &io, &fdie, sizeof fdie, FileDispositionInformationEx); /* Restore R/O attribute in case we have multiple hardlinks. */ - if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY) + if (access & FILE_WRITE_ATTRIBUTES) NtSetAttributesFile (fh, pc.file_attributes ()); NtClose (fh); /* Trying to delete in-use executables and DLLs using diff --git a/winsup/cygwin/wincap.cc b/winsup/cygwin/wincap.cc index b18e732cd..635e0892b 100644 --- a/winsup/cygwin/wincap.cc +++ b/winsup/cygwin/wincap.cc @@ -38,6 +38,7 @@ wincaps wincap_vista __attribute__((section (".cygwin_dll_common"), shared)) = { has_unbiased_interrupt_time:false, has_precise_interrupt_time:false, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:true, @@ -72,6 +73,7 @@ wincaps wincap_7 __attribute__((section (".cygwin_dll_common"), shared)) = { has_unbiased_interrupt_time:true, has_precise_interrupt_time:false, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:true, @@ -106,6 +108,7 @@ wincaps wincap_8 __attribute__((section (".cygwin_dll_common"), shared)) = { has_unbiased_interrupt_time:true, has_precise_interrupt_time:false, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -140,6 +143,7 @@ wincaps wincap_8_1 __attribute__((section (".cygwin_dll_common"), shared)) = { has_unbiased_interrupt_time:true, has_precise_interrupt_time:false, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -174,6 +178,7 @@ wincaps wincap_10_1507 __attribute__((section (".cygwin_dll_common"), shared)) has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -208,6 +213,7 @@ wincaps wincap_10_1607 __attribute__((section (".cygwin_dll_common"), shared)) has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -242,6 +248,7 @@ wincaps wincap_10_1703 __attribute__((section (".cygwin_dll_common"), shared)) = has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:false, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -276,6 +283,7 @@ wincaps wincap_10_1709 __attribute__((section (".cygwin_dll_common"), shared)) = has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:true, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:false, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -310,6 +318,7 @@ wincaps wincap_10_1803 __attribute__((section (".cygwin_dll_common"), shared)) = has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:true, + has_posix_unlink_semantics_with_ignore_readonly:false, has_case_sensitive_dirs:true, has_posix_rename_semantics:false, no_msv1_0_s4u_logon_in_wow64:false, @@ -344,6 +353,7 @@ wincaps wincap_10_1809 __attribute__((section (".cygwin_dll_common"), shared)) = has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:true, + has_posix_unlink_semantics_with_ignore_readonly:true, has_case_sensitive_dirs:true, has_posix_rename_semantics:true, no_msv1_0_s4u_logon_in_wow64:false, @@ -378,6 +388,7 @@ wincaps wincap_10_1903 __attribute__((section (".cygwin_dll_common"), shared)) = has_unbiased_interrupt_time:true, has_precise_interrupt_time:true, has_posix_unlink_semantics:true, + has_posix_unlink_semantics_with_ignore_readonly:true, has_case_sensitive_dirs:true, has_posix_rename_semantics:true, no_msv1_0_s4u_logon_in_wow64:false, diff --git a/winsup/cygwin/wincap.h b/winsup/cygwin/wincap.h index 2f4191aa1..6be2ca2a1 100644 --- a/winsup/cygwin/wincap.h +++ b/winsup/cygwin/wincap.h @@ -16,33 +16,34 @@ struct wincaps /* The bitfields must be 8 byte aligned on x86_64, otherwise the bitfield ops generated by gcc are off by 4 bytes. */ struct __attribute__ ((aligned (8))) { - unsigned is_server : 1; - unsigned needs_count_in_si_lpres2 : 1; - unsigned needs_query_information : 1; - unsigned has_gaa_largeaddress_bug : 1; - unsigned has_broken_alloc_console : 1; - unsigned has_console_logon_sid : 1; - unsigned has_precise_system_time : 1; - unsigned has_microsoft_accounts : 1; - unsigned has_processor_groups : 1; - unsigned has_broken_prefetchvm : 1; - unsigned has_new_pebteb_region : 1; - unsigned has_broken_whoami : 1; - unsigned has_unprivileged_createsymlink : 1; - unsigned has_unbiased_interrupt_time : 1; - unsigned has_precise_interrupt_time : 1; - unsigned has_posix_unlink_semantics : 1; - unsigned has_case_sensitive_dirs : 1; - unsigned has_posix_rename_semantics : 1; - unsigned no_msv1_0_s4u_logon_in_wow64 : 1; - unsigned has_con_24bit_colors : 1; - unsigned has_con_broken_csi3j : 1; - unsigned has_con_broken_il_dl : 1; - unsigned has_con_esc_rep : 1; - unsigned has_extended_mem_api : 1; - unsigned has_tcp_fastopen : 1; - unsigned has_linux_tcp_keepalive_sockopts : 1; - unsigned has_tcp_maxrtms : 1; + unsigned is_server : 1; + unsigned needs_count_in_si_lpres2 : 1; + unsigned needs_query_information : 1; + unsigned has_gaa_largeaddress_bug : 1; + unsigned has_broken_alloc_console : 1; + unsigned has_console_logon_sid : 1; + unsigned has_precise_system_time : 1; + unsigned has_microsoft_accounts : 1; + unsigned has_processor_groups : 1; + unsigned has_broken_prefetchvm : 1; + unsigned has_new_pebteb_region : 1; + unsigned has_broken_whoami : 1; + unsigned has_unprivileged_createsymlink : 1; + unsigned has_unbiased_interrupt_time : 1; + unsigned has_precise_interrupt_time : 1; + unsigned has_posix_unlink_semantics : 1; + unsigned has_posix_unlink_semantics_with_ignore_readonly : 1; + unsigned has_case_sensitive_dirs : 1; + unsigned has_posix_rename_semantics : 1; + unsigned no_msv1_0_s4u_logon_in_wow64 : 1; + unsigned has_con_24bit_colors : 1; + unsigned has_con_broken_csi3j : 1; + unsigned has_con_broken_il_dl : 1; + unsigned has_con_esc_rep : 1; + unsigned has_extended_mem_api : 1; + unsigned has_tcp_fastopen : 1; + unsigned has_linux_tcp_keepalive_sockopts : 1; + unsigned has_tcp_maxrtms : 1; }; }; @@ -98,6 +99,7 @@ public: bool IMPLEMENT (has_unbiased_interrupt_time) bool IMPLEMENT (has_precise_interrupt_time) bool IMPLEMENT (has_posix_unlink_semantics) + bool IMPLEMENT (has_posix_unlink_semantics_with_ignore_readonly) bool IMPLEMENT (has_case_sensitive_dirs) bool IMPLEMENT (has_posix_rename_semantics) bool IMPLEMENT (no_msv1_0_s4u_logon_in_wow64) -- 2.43.5