You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/02/14 23:40:53 UTC
Review Request 56689: Windows: Prevented crash if sandbox `stdout`
file already exists.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56689/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Repository: mesos
Description
-------
Subprocess currently provides a cross-platform API that allows the child
process to redirect `stdin`, `stdout`, and `stderr`, with support for
file paths, file descriptors, pipes, or Windows file `HANDLE`s.
Currently, in the case of file paths, the Windows code will use
`CreateFile` to open the file, with the semantics that it will error out
if the file already exists (because of the `CREATE_NEW` flag), and
explicitly specifying that it is ok to have multiple processes writing
to the file (because of the `FILE_SHARE_WRITE` flag).
The former of these is undesirable; libprocess should be able to proceed
regardless of whether the file already exists. But this also causes bugs
to the downstream, namely Mesos, in which a Fetcher might create the
sandbox `stdout` folder, and then subsequently crash when it tries to
open the `stdout` folder in the executor.
In this commit, we change the `CREATE_NEW` flag to `OPEN_ALWAYS`, which
has the semantics that we will create the file if it does not exist, and
open it if it does.
Additionally, since the documentation does not specify whether
`FILE_SHARE_WRITE` also allows other processes to have read access, we
additionally specify the flag `FILE_SHARE_READ` just to be sure we make
no assumptions about who is able to read and write to these files. See
comments for additional context on this point.
Diffs
-----
3rdparty/libprocess/src/subprocess_windows.cpp 1b5ae5e3ee3c08d05afa1bc533542f42fe26eed0
Diff: https://reviews.apache.org/r/56689/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 56689: Windows: Prevented crash if sandbox
`stdout` file already exists.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56689/#review168509
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On Feb. 14, 2017, 3:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56689/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:40 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Subprocess currently provides a cross-platform API that allows the child
> process to redirect `stdin`, `stdout`, and `stderr`, with support for
> file paths, file descriptors, pipes, or Windows file `HANDLE`s.
>
> Currently, in the case of file paths, the Windows code will use
> `CreateFile` to open the file, with the semantics that it will error out
> if the file already exists (because of the `CREATE_NEW` flag), and
> explicitly specifying that it is ok to have multiple processes writing
> to the file (because of the `FILE_SHARE_WRITE` flag).
>
> The former of these is undesirable; libprocess should be able to proceed
> regardless of whether the file already exists. But this also causes bugs
> to the downstream, namely Mesos, in which a Fetcher might create the
> sandbox `stdout` folder, and then subsequently crash when it tries to
> open the `stdout` folder in the executor.
>
> In this commit, we change the `CREATE_NEW` flag to `OPEN_ALWAYS`, which
> has the semantics that we will create the file if it does not exist, and
> open it if it does.
>
> Additionally, since the documentation does not specify whether
> `FILE_SHARE_WRITE` also allows other processes to have read access, we
> additionally specify the flag `FILE_SHARE_READ` just to be sure we make
> no assumptions about who is able to read and write to these files. See
> comments for additional context on this point.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_windows.cpp 1b5ae5e3ee3c08d05afa1bc533542f42fe26eed0
>
>
> Diff: https://reviews.apache.org/r/56689/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 56689: Windows: Prevented crash if sandbox
`stdout` file already exists.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56689/#review165642
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55022, 55023, 55024, 55030, 55162, 55543, 55546, 55547, 55549, 55550, 55749, 56504, 56505, 56591, 56592, 56593, 56594, 56652, 56675, 56689]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 14, 2017, 11:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56689/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 11:40 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Subprocess currently provides a cross-platform API that allows the child
> process to redirect `stdin`, `stdout`, and `stderr`, with support for
> file paths, file descriptors, pipes, or Windows file `HANDLE`s.
>
> Currently, in the case of file paths, the Windows code will use
> `CreateFile` to open the file, with the semantics that it will error out
> if the file already exists (because of the `CREATE_NEW` flag), and
> explicitly specifying that it is ok to have multiple processes writing
> to the file (because of the `FILE_SHARE_WRITE` flag).
>
> The former of these is undesirable; libprocess should be able to proceed
> regardless of whether the file already exists. But this also causes bugs
> to the downstream, namely Mesos, in which a Fetcher might create the
> sandbox `stdout` folder, and then subsequently crash when it tries to
> open the `stdout` folder in the executor.
>
> In this commit, we change the `CREATE_NEW` flag to `OPEN_ALWAYS`, which
> has the semantics that we will create the file if it does not exist, and
> open it if it does.
>
> Additionally, since the documentation does not specify whether
> `FILE_SHARE_WRITE` also allows other processes to have read access, we
> additionally specify the flag `FILE_SHARE_READ` just to be sure we make
> no assumptions about who is able to read and write to these files. See
> comments for additional context on this point.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_windows.cpp 1b5ae5e3ee3c08d05afa1bc533542f42fe26eed0
>
> Diff: https://reviews.apache.org/r/56689/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>