[v3,42/54] chardev/char-file: Add FILE_SHARE_WRITE when opening the file for win32

Commit Message

Bin Meng Sept. 25, 2022, 11:30 a.m. UTC

From: Xuzhou Cheng <[e-mail]>

The combination of GENERIC_WRITE and FILE_SHARE_READ options does not
allow the same file to be opened again by CreateFile() from another
QEMU process with the same options when the previous QEMU process
still holds the file handle opened.

This was triggered by running the test_multifd_tcp_cancel() case on
Windows, which cancels the migration, and launches another QEMU
process to migrate with the same file opened for write. Chances are
that the previous QEMU process does not quit before the new QEMU
process runs hence the old one still holds the file handle that does
not allow shared write permission then the new QEMU process will fail.

There is another test case boot-serial-test that triggers the same
issue. The qtest executable created a serial chardev file to be
passed to the QEMU executable. The serial file was created by
g_file_open_tmp(), which internally opens the file with
FILE_SHARE_WRITE security attribute, and based on [1], there is
only one case that allows the first call to CreateFile() with
GENERIC_READ & FILE_SHARE_WRITE, and second call to CreateFile()
with GENERIC_WRITE & FILE_SHARE_READ. All other combinations
require FILE_SHARE_WRITE in the second call. But there is no way
for the second call (in this case the QEMU executable) to know
what combination was passed to the first call, so we will have to
add FILE_SHARE_WRITE to the second call.

For both scenarios we should add FILE_SHARE_WRITE in the chardev
file backend driver. This change also makes the behavior to be
consistent with the POSIX platforms.

[1] https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files

Signed-off-by: Xuzhou Cheng <[e-mail]>
Signed-off-by: Bin Meng <[e-mail]>
---

Changes in v3:
- Add another case "boot-serial-test" to justify the change

Changes in v2:
- Update commit message to include the use case why we should set
  FILE_SHARE_WRITE when opening the file for win32

 chardev/char-file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Marc-André Lureau Sept. 26, 2022, 1:27 p.m. UTC | #1

Hi

On Sun, Sep 25, 2022 at 4:35 PM Bin Meng <[e-mail]> wrote:

> From: Xuzhou Cheng <[e-mail]>
>
> The combination of GENERIC_WRITE and FILE_SHARE_READ options does not
> allow the same file to be opened again by CreateFile() from another
> QEMU process with the same options when the previous QEMU process
> still holds the file handle opened.
>
> This was triggered by running the test_multifd_tcp_cancel() case on
> Windows, which cancels the migration, and launches another QEMU
> process to migrate with the same file opened for write. Chances are
> that the previous QEMU process does not quit before the new QEMU
> process runs hence the old one still holds the file handle that does
> not allow shared write permission then the new QEMU process will fail.
>
> There is another test case boot-serial-test that triggers the same
> issue. The qtest executable created a serial chardev file to be
> passed to the QEMU executable. The serial file was created by
> g_file_open_tmp(), which internally opens the file with
> FILE_SHARE_WRITE security attribute, and based on [1], there is
> only one case that allows the first call to CreateFile() with
> GENERIC_READ & FILE_SHARE_WRITE, and second call to CreateFile()
> with GENERIC_WRITE & FILE_SHARE_READ. All other combinations
> require FILE_SHARE_WRITE in the second call. But there is no way
> for the second call (in this case the QEMU executable) to know
> what combination was passed to the first call, so we will have to
> add FILE_SHARE_WRITE to the second call.
>
> For both scenarios we should add FILE_SHARE_WRITE in the chardev
> file backend driver. This change also makes the behavior to be
> consistent with the POSIX platforms.
>

It seems to me the tests should be fixed instead. I thought you fixed the
first case. For the second case, why not close the file before starting
qemu? If you have issues, I will take a deeper look.


>
> [1]
> https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files
>
> Signed-off-by: Xuzhou Cheng <[e-mail]>
> Signed-off-by: Bin Meng <[e-mail]>
> ---
>
> Changes in v3:
> - Add another case "boot-serial-test" to justify the change
>
> Changes in v2:
> - Update commit message to include the use case why we should set
>   FILE_SHARE_WRITE when opening the file for win32
>
>  chardev/char-file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-file.c b/chardev/char-file.c
> index 2fd80707e5..66385211eb 100644
> --- a/chardev/char-file.c
> +++ b/chardev/char-file.c
> @@ -60,8 +60,8 @@ static void qmp_chardev_open_file(Chardev *chr,
>          flags = CREATE_ALWAYS;
>      }
>
> -    out = CreateFile(file->out, accessmode, FILE_SHARE_READ, NULL, flags,
> -                     FILE_ATTRIBUTE_NORMAL, NULL);
> +    out = CreateFile(file->out, accessmode, FILE_SHARE_READ |
> FILE_SHARE_WRITE,
> +                     NULL, flags, FILE_ATTRIBUTE_NORMAL, NULL);
>      if (out == INVALID_HANDLE_VALUE) {
>          error_setg(errp, "open %s failed", file->out);
>          return;
> --
> 2.34.1
>
>
>

Bin Meng Sept. 26, 2022, 3:05 p.m. UTC | #2

On Mon, Sep 26, 2022 at 9:27 PM Marc-André Lureau
<[e-mail]> wrote:
>
> Hi
>
> On Sun, Sep 25, 2022 at 4:35 PM Bin Meng <[e-mail]> wrote:
>>
>> From: Xuzhou Cheng <[e-mail]>
>>
>> The combination of GENERIC_WRITE and FILE_SHARE_READ options does not
>> allow the same file to be opened again by CreateFile() from another
>> QEMU process with the same options when the previous QEMU process
>> still holds the file handle opened.
>>
>> This was triggered by running the test_multifd_tcp_cancel() case on
>> Windows, which cancels the migration, and launches another QEMU
>> process to migrate with the same file opened for write. Chances are
>> that the previous QEMU process does not quit before the new QEMU
>> process runs hence the old one still holds the file handle that does
>> not allow shared write permission then the new QEMU process will fail.
>>
>> There is another test case boot-serial-test that triggers the same
>> issue. The qtest executable created a serial chardev file to be
>> passed to the QEMU executable. The serial file was created by
>> g_file_open_tmp(), which internally opens the file with
>> FILE_SHARE_WRITE security attribute, and based on [1], there is
>> only one case that allows the first call to CreateFile() with
>> GENERIC_READ & FILE_SHARE_WRITE, and second call to CreateFile()
>> with GENERIC_WRITE & FILE_SHARE_READ. All other combinations
>> require FILE_SHARE_WRITE in the second call. But there is no way
>> for the second call (in this case the QEMU executable) to know
>> what combination was passed to the first call, so we will have to
>> add FILE_SHARE_WRITE to the second call.
>>
>> For both scenarios we should add FILE_SHARE_WRITE in the chardev
>> file backend driver. This change also makes the behavior to be
>> consistent with the POSIX platforms.
>
>
> It seems to me the tests should be fixed instead. I thought you fixed the first case. For the second case, why not close the file before starting qemu? If you have issues, I will take a deeper look.

Indeed, the following test case change can "fix" this issue:

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 72310ba30e..f192fbc181 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -233,6 +233,7 @@ static void test_machine(const void *data)
ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL);
g_assert(ser_fd != -1);
+ close(ser_fd);
if (test->kernel) {
code = test->kernel;
@@ -266,6 +267,7 @@ static void test_machine(const void *data)
unlink(codetmp);
}
+ ser_fd = open(serialtmp, O_RDONLY);
if (!check_guest_output(qts, test, ser_fd)) {
g_error("Failed to find expected string. Please check '%s'",
serialtmp);

But I think it just workarounds the problem. The original test case
looks reasonable to me. If we update the case like above, we cannot
guarantee users will do like the updated test case does.

>
>>
>>
>> [1] https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files
>>
>> Signed-off-by: Xuzhou Cheng <[e-mail]>
>> Signed-off-by: Bin Meng <[e-mail]>
>> ---
>>
>> Changes in v3:
>> - Add another case "boot-serial-test" to justify the change
>>
>> Changes in v2:
>> - Update commit message to include the use case why we should set
>>   FILE_SHARE_WRITE when opening the file for win32
>>
>>  chardev/char-file.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>

Regards,
Bin

Marc-André Lureau Sept. 27, 2022, 9 a.m. UTC | #3

Hi

On Mon, Sep 26, 2022 at 7:05 PM Bin Meng <[e-mail]> wrote:

> On Mon, Sep 26, 2022 at 9:27 PM Marc-André Lureau
> <[e-mail]> wrote:
> >
> > Hi
> >
> > On Sun, Sep 25, 2022 at 4:35 PM Bin Meng <[e-mail]> wrote:
> >>
> >> From: Xuzhou Cheng <[e-mail]>
> >>
> >> The combination of GENERIC_WRITE and FILE_SHARE_READ options does not
> >> allow the same file to be opened again by CreateFile() from another
> >> QEMU process with the same options when the previous QEMU process
> >> still holds the file handle opened.
> >>
> >> This was triggered by running the test_multifd_tcp_cancel() case on
> >> Windows, which cancels the migration, and launches another QEMU
> >> process to migrate with the same file opened for write. Chances are
> >> that the previous QEMU process does not quit before the new QEMU
> >> process runs hence the old one still holds the file handle that does
> >> not allow shared write permission then the new QEMU process will fail.
> >>
> >> There is another test case boot-serial-test that triggers the same
> >> issue. The qtest executable created a serial chardev file to be
> >> passed to the QEMU executable. The serial file was created by
> >> g_file_open_tmp(), which internally opens the file with
> >> FILE_SHARE_WRITE security attribute, and based on [1], there is
> >> only one case that allows the first call to CreateFile() with
> >> GENERIC_READ & FILE_SHARE_WRITE, and second call to CreateFile()
> >> with GENERIC_WRITE & FILE_SHARE_READ. All other combinations
> >> require FILE_SHARE_WRITE in the second call. But there is no way
> >> for the second call (in this case the QEMU executable) to know
> >> what combination was passed to the first call, so we will have to
> >> add FILE_SHARE_WRITE to the second call.
> >>
> >> For both scenarios we should add FILE_SHARE_WRITE in the chardev
> >> file backend driver. This change also makes the behavior to be
> >> consistent with the POSIX platforms.
> >
> >
> > It seems to me the tests should be fixed instead. I thought you fixed
> the first case. For the second case, why not close the file before starting
> qemu? If you have issues, I will take a deeper look.
>
> Indeed, the following test case change can "fix" this issue:
>
> diff --git a/tests/qtest/boot-serial-test.c
> b/tests/qtest/boot-serial-test.c
> index 72310ba30e..f192fbc181 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -233,6 +233,7 @@ static void test_machine(const void *data)
> ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL);
> g_assert(ser_fd != -1);
> + close(ser_fd);
> if (test->kernel) {
> code = test->kernel;
> @@ -266,6 +267,7 @@ static void test_machine(const void *data)
> unlink(codetmp);
> }
> + ser_fd = open(serialtmp, O_RDONLY);
> if (!check_guest_output(qts, test, ser_fd)) {
> g_error("Failed to find expected string. Please check '%s'",
> serialtmp);
>
>
Please send this fix as a new patch in the series.


> But I think it just workarounds the problem. The original test case
> looks reasonable to me. If we update the case like above, we cannot
> guarantee users will do like the updated test case does.
>

If the test is enabled, it will fail, and the reasons are reasonably valid:
two processes shouldn't share the same file for writing with a chardev.

I still think the windows file chardev behavior is superior and we should
instead teach the posix implementation of exclusive write access, rather
than downgrading the windows implementation. I'd drop this patch from the
series for now.


>
> >
> >>
> >>
> >> [1]
> https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files
> >>
> >> Signed-off-by: Xuzhou Cheng <[e-mail]>
> >> Signed-off-by: Bin Meng <[e-mail]>
> >> ---
> >>
> >> Changes in v3:
> >> - Add another case "boot-serial-test" to justify the change
> >>
> >> Changes in v2:
> >> - Update commit message to include the use case why we should set
> >>   FILE_SHARE_WRITE when opening the file for win32
> >>
> >>  chardev/char-file.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
>
> Regards,
> Bin
>

Bin Meng Sept. 27, 2022, 10:44 a.m. UTC | #4

On Tue, Sep 27, 2022 at 5:00 PM Marc-André Lureau
<[e-mail]> wrote:
>
> Hi
>
> On Mon, Sep 26, 2022 at 7:05 PM Bin Meng <[e-mail]> wrote:
>>
>> On Mon, Sep 26, 2022 at 9:27 PM Marc-André Lureau
>> <[e-mail]> wrote:
>> >
>> > Hi
>> >
>> > On Sun, Sep 25, 2022 at 4:35 PM Bin Meng <[e-mail]> wrote:
>> >>
>> >> From: Xuzhou Cheng <[e-mail]>
>> >>
>> >> The combination of GENERIC_WRITE and FILE_SHARE_READ options does not
>> >> allow the same file to be opened again by CreateFile() from another
>> >> QEMU process with the same options when the previous QEMU process
>> >> still holds the file handle opened.
>> >>
>> >> This was triggered by running the test_multifd_tcp_cancel() case on
>> >> Windows, which cancels the migration, and launches another QEMU
>> >> process to migrate with the same file opened for write. Chances are
>> >> that the previous QEMU process does not quit before the new QEMU
>> >> process runs hence the old one still holds the file handle that does
>> >> not allow shared write permission then the new QEMU process will fail.
>> >>
>> >> There is another test case boot-serial-test that triggers the same
>> >> issue. The qtest executable created a serial chardev file to be
>> >> passed to the QEMU executable. The serial file was created by
>> >> g_file_open_tmp(), which internally opens the file with
>> >> FILE_SHARE_WRITE security attribute, and based on [1], there is
>> >> only one case that allows the first call to CreateFile() with
>> >> GENERIC_READ & FILE_SHARE_WRITE, and second call to CreateFile()
>> >> with GENERIC_WRITE & FILE_SHARE_READ. All other combinations
>> >> require FILE_SHARE_WRITE in the second call. But there is no way
>> >> for the second call (in this case the QEMU executable) to know
>> >> what combination was passed to the first call, so we will have to
>> >> add FILE_SHARE_WRITE to the second call.
>> >>
>> >> For both scenarios we should add FILE_SHARE_WRITE in the chardev
>> >> file backend driver. This change also makes the behavior to be
>> >> consistent with the POSIX platforms.
>> >
>> >
>> > It seems to me the tests should be fixed instead. I thought you fixed the first case. For the second case, why not close the file before starting qemu? If you have issues, I will take a deeper look.
>>
>> Indeed, the following test case change can "fix" this issue:
>>
>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
>> index 72310ba30e..f192fbc181 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -233,6 +233,7 @@ static void test_machine(const void *data)
>> ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL);
>> g_assert(ser_fd != -1);
>> + close(ser_fd);
>> if (test->kernel) {
>> code = test->kernel;
>> @@ -266,6 +267,7 @@ static void test_machine(const void *data)
>> unlink(codetmp);
>> }
>> + ser_fd = open(serialtmp, O_RDONLY);
>> if (!check_guest_output(qts, test, ser_fd)) {
>> g_error("Failed to find expected string. Please check '%s'",
>> serialtmp);
>>
>
> Please send this fix as a new patch in the series.
>
>>
>> But I think it just workarounds the problem. The original test case
>> looks reasonable to me. If we update the case like above, we cannot
>> guarantee users will do like the updated test case does.
>
>
> If the test is enabled, it will fail, and the reasons are reasonably valid: two processes shouldn't share the same file for writing with a chardev.
>
> I still think the windows file chardev behavior is superior and we should instead teach the posix implementation of exclusive write access, rather than downgrading the windows implementation. I'd drop this patch from the series for now.
>

Okay, will drop this patch, and add the test case fix as a new patch in v4.

Regards,
Bin

Patch

diff --git a/chardev/char-file.c b/chardev/char-file.c
index 2fd80707e5..66385211eb 100644
--- a/chardev/char-file.c
+++ b/chardev/char-file.c
@@ -60,8 +60,8 @@  static void qmp_chardev_open_file(Chardev *chr,
         flags = CREATE_ALWAYS;
     }
 
-    out = CreateFile(file->out, accessmode, FILE_SHARE_READ, NULL, flags,
-                     FILE_ATTRIBUTE_NORMAL, NULL);
+    out = CreateFile(file->out, accessmode, FILE_SHARE_READ | FILE_SHARE_WRITE,
+                     NULL, flags, FILE_ATTRIBUTE_NORMAL, NULL);
     if (out == INVALID_HANDLE_VALUE) {
         error_setg(errp, "open %s failed", file->out);
         return;