From 7a2c889fa8bf45587ee3efdeccf48ec22781b84b Mon Sep 17 00:00:00 2001
From: Joran Dirk Greef <joran@ronomon.com>
Date: Fri, 30 Nov 2018 16:35:31 +0200
Subject: [PATCH] win: fs: fix `FILE_FLAG_NO_BUFFERING` for writes

On Windows, `fs__open()` maps `UV_FS_O_DIRECT` to
`FILE_FLAG_NO_BUFFERING`.

When `access` is only `FILE_GENERIC_READ` this succeeds, but when
`access` is `FILE_GENERIC_WRITE` this returns an error:

```
0x00000057, ERROR_INVALID_PARAMETER, The parameter is incorrect.
```

The reason is that `FILE_GENERIC_WRITE` includes `FILE_APPEND_DATA`,
but `FILE_APPEND_DATA` and `FILE_FLAG_NO_BUFFERING` are mutually
exclusive:

```
FILE_GENERIC_WRITE = STANDARD_RIGHTS_WRITE |
                     FILE_WRITE_DATA |
                     FILE_WRITE_ATTRIBUTES |
                     FILE_WRITE_EA |
                     FILE_APPEND_DATA |
                     SYNCHRONIZE
```

This incompatibility between access and attribute flags does not appear
to be documented by Microsoft for `FILE_FLAG_NO_BUFFERING` but it is
indirectly documented under [NtCreateFile](https://bit.ly/2rm5wRT):

```
FILE_NO_INTERMEDIATE_BUFFERING
The file cannot be cached or buffered in a driver's internal buffers.
This flag is incompatible with the DesiredAccess FILE_APPEND_DATA flag.
```

The solution is to remove `FILE_APPEND_DATA` from the access flags when
`FILE_FLAG_NO_BUFFERING` is set. Note that this does not prevent
appends, since `FILE_GENERIC_WRITE` also includes `FILE_WRITE_DATA`,
which in turn allows appends.

PR-URL: https://github.com/libuv/libuv/pull/2102
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 src/win/fs.c     | 27 +++++++++++++++++++++++++++
 test/test-fs.c   | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 test/test-list.h |  2 ++
 3 files changed, 76 insertions(+)

diff --git a/src/win/fs.c b/src/win/fs.c
index a7dfb7c5feb..0716ecca122 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -510,6 +510,33 @@ void fs__open(uv_fs_t* req) {
   }
 
   if (flags & UV_FS_O_DIRECT) {
+    /*
+     * FILE_APPEND_DATA and FILE_FLAG_NO_BUFFERING are mutually exclusive.
+     * Windows returns 87, ERROR_INVALID_PARAMETER if these are combined.
+     *
+     * FILE_APPEND_DATA is included in FILE_GENERIC_WRITE:
+     *
+     * FILE_GENERIC_WRITE = STANDARD_RIGHTS_WRITE |
+     *                      FILE_WRITE_DATA |
+     *                      FILE_WRITE_ATTRIBUTES |
+     *                      FILE_WRITE_EA |
+     *                      FILE_APPEND_DATA |
+     *                      SYNCHRONIZE
+     *
+     * Note: Appends are also permitted by FILE_WRITE_DATA.
+     *
+     * In order for direct writes and direct appends to succeed, we therefore
+     * exclude FILE_APPEND_DATA if FILE_WRITE_DATA is specified, and otherwise
+     * fail if the user's sole permission is a direct append, since this
+     * particular combination is invalid.
+     */
+    if (access & FILE_APPEND_DATA) {
+      if (access & FILE_WRITE_DATA) {
+        access &= ~FILE_APPEND_DATA;
+      } else {
+        goto einval;
+      }
+    }
     attributes |= FILE_FLAG_NO_BUFFERING;
   }
 
diff --git a/test/test-fs.c b/test/test-fs.c
index 5530b3c864c..5eed160de2e 100644
--- a/test/test-fs.c
+++ b/test/test-fs.c
@@ -3600,6 +3600,53 @@ TEST_IMPL(fs_exclusive_sharing_mode) {
 }
 #endif
 
+#ifdef _WIN32
+TEST_IMPL(fs_file_flag_no_buffering) {
+  int r;
+
+  /* Setup. */
+  unlink("test_file");
+
+  ASSERT(UV_FS_O_APPEND > 0);
+  ASSERT(UV_FS_O_CREAT > 0);
+  ASSERT(UV_FS_O_DIRECT > 0);
+  ASSERT(UV_FS_O_RDWR > 0);
+
+  /* FILE_APPEND_DATA must be excluded from FILE_GENERIC_WRITE: */
+  r = uv_fs_open(NULL,
+                 &open_req1,
+                 "test_file",
+                 UV_FS_O_RDWR | UV_FS_O_CREAT | UV_FS_O_DIRECT,
+                 S_IWUSR | S_IRUSR,
+                 NULL);
+  ASSERT(r >= 0);
+  ASSERT(open_req1.result >= 0);
+  uv_fs_req_cleanup(&open_req1);
+
+  r = uv_fs_close(NULL, &close_req, open_req1.result, NULL);
+  ASSERT(r == 0);
+  ASSERT(close_req.result == 0);
+  uv_fs_req_cleanup(&close_req);
+
+  /* FILE_APPEND_DATA and FILE_FLAG_NO_BUFFERING are mutually exclusive: */
+  r = uv_fs_open(NULL,
+                 &open_req2,
+                 "test_file",
+                 UV_FS_O_APPEND | UV_FS_O_DIRECT,
+                 S_IWUSR | S_IRUSR,
+                 NULL);
+  ASSERT(r == UV_EINVAL);
+  ASSERT(open_req2.result == UV_EINVAL);
+  uv_fs_req_cleanup(&open_req2);
+
+  /* Cleanup */
+  unlink("test_file");
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
+#endif
+
 #ifdef _WIN32
 int call_icacls(const char* command, ...) {
     char icacls_command[1024];
diff --git a/test/test-list.h b/test/test-list.h
index 7ff5dab9787..cc37bc039b1 100644
--- a/test/test-list.h
+++ b/test/test-list.h
@@ -349,6 +349,7 @@ TEST_DECLARE   (fs_null_req)
 TEST_DECLARE   (fs_read_dir)
 #ifdef _WIN32
 TEST_DECLARE   (fs_exclusive_sharing_mode)
+TEST_DECLARE   (fs_file_flag_no_buffering)
 TEST_DECLARE   (fs_open_readonly_acl)
 TEST_DECLARE   (fs_fchmod_archive_readonly)
 #endif
@@ -905,6 +906,7 @@ TASK_LIST_START
   TEST_ENTRY  (fs_read_dir)
 #ifdef _WIN32
   TEST_ENTRY  (fs_exclusive_sharing_mode)
+  TEST_ENTRY  (fs_file_flag_no_buffering)
   TEST_ENTRY  (fs_open_readonly_acl)
   TEST_ENTRY  (fs_fchmod_archive_readonly)
 #endif