From 132268042d1992d39614e72d67957b2b10ebfba6 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sun, 6 Sep 2009 14:45:42 -0400 Subject: [PATCH] Windows: Prevent simultaneous pioctls The Windows pioctl implementation makes an incorrect assumption. It is not true that every CreateFile() operation results in a SMB NTCreateX operation being delivered to SMB Server. The SMB client can combine open requests from multiple processes or threads onto a single SMB file descriptor and locally manage the operations. This is a problem for pioctls since the Transceive operation requires that a WriteFile/ReadFile combination must belong to the same request. Prior to this change simultaneous pioctl operations would be combined and the individual reads and writes could overlap resulting in responses going to the wrong requestor and end of file errors being received by the others. Due to lack of data validation in fs.c, ktc_nt.c, symlink.c, etc random crashes are produced. This change alters the sharing mode under which the pioctl file is opened. Instead of FILE_SHARE_READ | FILE_SHARE_WRITE, only FILE_SHARE_READ is specified to CreateFile(). This ensures that the CreateFile will fail with a sharing violation if the pioctl file was previously opened for writing. A sharing violation check is provided and the CreateFile is retried indefinitely until the open succeeds or the error is not a sharing violation. LICENSE MIT Reviewed-on: http://gerrit.openafs.org/404 Reviewed-by: Derrick Brashear Tested-by: Asanka Herath Reviewed-by: Asanka Herath Tested-by: Jeffrey Altman Reviewed-by: Jeffrey Altman --- src/sys/pioctl_nt.c | 52 +++++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/src/sys/pioctl_nt.c b/src/sys/pioctl_nt.c index db2acf3bd..a4da876c3 100644 --- a/src/sys/pioctl_nt.c +++ b/src/sys/pioctl_nt.c @@ -594,6 +594,7 @@ GetIoctlHandle(char *fileNamep, HANDLE * handlep) DWORD dwSize = sizeof(szUser); int saveerrno; UINT driveType; + int sharingViolation; memset(HostName, '\0', sizeof(HostName)); gethostname(HostName, sizeof(HostName)); @@ -695,11 +696,16 @@ GetIoctlHandle(char *fileNamep, HANDLE * handlep) fflush(stdout); /* now open the file */ - fh = CreateFile(tbuffer, GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, - FILE_FLAG_WRITE_THROUGH, NULL); - - fflush(stdout); + sharingViolation = 0; + do { + if (sharingViolation) + Sleep(1); + fh = CreateFile(tbuffer, FILE_READ_DATA | FILE_WRITE_DATA, + FILE_SHARE_READ, NULL, OPEN_EXISTING, + FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 1; + } while (fh == INVALID_HANDLE_VALUE && GetLastError() == ERROR_SHARING_VIOLATION); + fflush(stdout); if (fh == INVALID_HANDLE_VALUE) { int gonext = 0; @@ -771,9 +777,15 @@ GetIoctlHandle(char *fileNamep, HANDLE * handlep) if (gonext) goto try_lsa_principal; - fh = CreateFile(tbuffer, GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, - FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 0; + do { + if (sharingViolation) + Sleep(1); + fh = CreateFile(tbuffer, FILE_READ_DATA | FILE_WRITE_DATA, + FILE_SHARE_READ, NULL, OPEN_EXISTING, + FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 1; + } while (fh == INVALID_HANDLE_VALUE && GetLastError() == ERROR_SHARING_VIOLATION); fflush(stdout); if (fh == INVALID_HANDLE_VALUE) { gle = GetLastError(); @@ -841,9 +853,15 @@ GetIoctlHandle(char *fileNamep, HANDLE * handlep) if (gonext) goto try_sam_compat; - fh = CreateFile(tbuffer, GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, - FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 0; + do { + if (sharingViolation) + Sleep(1); + fh = CreateFile(tbuffer, FILE_READ_DATA | FILE_WRITE_DATA, + FILE_SHARE_READ, NULL, OPEN_EXISTING, + FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 1; + } while (fh == INVALID_HANDLE_VALUE && GetLastError() == ERROR_SHARING_VIOLATION); fflush(stdout); if (fh == INVALID_HANDLE_VALUE) { gle = GetLastError(); @@ -906,9 +924,15 @@ GetIoctlHandle(char *fileNamep, HANDLE * handlep) return -1; } - fh = CreateFile(tbuffer, GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, - FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 0; + do { + if (sharingViolation) + Sleep(1); + fh = CreateFile(tbuffer, FILE_READ_DATA | FILE_WRITE_DATA, + FILE_SHARE_READ, NULL, OPEN_EXISTING, + FILE_FLAG_WRITE_THROUGH, NULL); + sharingViolation = 1; + } while (fh == INVALID_HANDLE_VALUE && GetLastError() == ERROR_SHARING_VIOLATION); fflush(stdout); if (fh == INVALID_HANDLE_VALUE) { gle = GetLastError(); -- 2.39.5