You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/01/19 02:41:18 UTC
Review Request 55701: Fixed unsafe usage of process pointer in
async.hpp.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55701/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Hindman, Benjamin Mahler, and Michael Park.
Repository: mesos
Description
-------
The `async(...)` helper spawns a libprocess actor to execute some
lambda (asynchronously, of course). This actor is owned by the
libprocess GC actor, but the `AsyncExecutor` stores a copy of that
pointer and dereferences it in several possible locations.
Instead, the `AsyncExecutor` should store the `PID` of the actor,
which can be safely used, even if the actor has already terminated;
such as turning libprocess finalization.
Diffs
-----
3rdparty/libprocess/include/process/async.hpp 8565a52c6ba4008edb852e898b8f0b6d598a194f
Diff: https://reviews.apache.org/r/55701/diff/
Testing
-------
Applied the following change:
```
diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
index e035a4e..4c60ef9 100644
--- a/src/launcher/executor.cpp
+++ b/src/launcher/executor.cpp
@@ -936,6 +930,8 @@ int main(int argc, char** argv)
process::spawn(executor.get());
process::wait(executor.get());
+ executor.reset();
+ process::finalize(true);
return EXIT_SUCCESS;
}
```
And then ran:
```
make check GTEST_FILTER="HTTPCommandExecutorTest.TerminateWithACK"
```
Thanks,
Joseph Wu
Re: Review Request 55701: Fixed unsafe usage of process pointer in
async.hpp.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55701/#review162234
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On Jan. 19, 2017, 2:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55701/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 2:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `async(...)` helper spawns a libprocess actor to execute some
> lambda (asynchronously, of course). This actor is owned by the
> libprocess GC actor, but the `AsyncExecutor` stores a copy of that
> pointer and dereferences it in several possible locations.
>
> Instead, the `AsyncExecutor` should store the `PID` of the actor,
> which can be safely used, even if the actor has already terminated;
> such as turning libprocess finalization.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/async.hpp 8565a52c6ba4008edb852e898b8f0b6d598a194f
>
> Diff: https://reviews.apache.org/r/55701/diff/
>
>
> Testing
> -------
>
> Applied the following change:
> ```
> diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
> index e035a4e..4c60ef9 100644
> --- a/src/launcher/executor.cpp
> +++ b/src/launcher/executor.cpp
> @@ -936,6 +930,8 @@ int main(int argc, char** argv)
>
> process::spawn(executor.get());
> process::wait(executor.get());
> + executor.reset();
>
> + process::finalize(true);
> return EXIT_SUCCESS;
> }
> ```
>
> And then ran:
> ```
> make check GTEST_FILTER="HTTPCommandExecutorTest.TerminateWithACK"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55701: Fixed unsafe usage of process pointer in
async.hpp.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 19, 2017, 6:08 a.m., Benjamin Bannier wrote:
> > Looking through other instances of `spawn(.*,\ true)`, should this one also be adjusted, https://github.com/apache/mesos/blob/745b3c7589e5252cf93f62e081b78fa420771d0c/3rdparty/libprocess/include/process/loop.hpp#L134-L144?
I'll open a separate review for this one. (I haven't hit that particular segfault in tests.)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55701/#review162283
-----------------------------------------------------------
On Jan. 18, 2017, 6:41 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55701/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 6:41 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `async(...)` helper spawns a libprocess actor to execute some
> lambda (asynchronously, of course). This actor is owned by the
> libprocess GC actor, but the `AsyncExecutor` stores a copy of that
> pointer and dereferences it in several possible locations.
>
> Instead, the `AsyncExecutor` should store the `PID` of the actor,
> which can be safely used, even if the actor has already terminated;
> such as turning libprocess finalization.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/async.hpp 8565a52c6ba4008edb852e898b8f0b6d598a194f
>
> Diff: https://reviews.apache.org/r/55701/diff/
>
>
> Testing
> -------
>
> Applied the following change:
> ```
> diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
> index e035a4e..4c60ef9 100644
> --- a/src/launcher/executor.cpp
> +++ b/src/launcher/executor.cpp
> @@ -936,6 +930,8 @@ int main(int argc, char** argv)
>
> process::spawn(executor.get());
> process::wait(executor.get());
> + executor.reset();
>
> + process::finalize(true);
> return EXIT_SUCCESS;
> }
> ```
>
> And then ran:
> ```
> make check GTEST_FILTER="HTTPCommandExecutorTest.TerminateWithACK"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55701: Fixed unsafe usage of process pointer in
async.hpp.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55701/#review162283
-----------------------------------------------------------
Ship it!
Looking through other instances of `spawn(.*,\ true)`, should this one also be adjusted, https://github.com/apache/mesos/blob/745b3c7589e5252cf93f62e081b78fa420771d0c/3rdparty/libprocess/include/process/loop.hpp#L134-L144?
- Benjamin Bannier
On Jan. 19, 2017, 3:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55701/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 3:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `async(...)` helper spawns a libprocess actor to execute some
> lambda (asynchronously, of course). This actor is owned by the
> libprocess GC actor, but the `AsyncExecutor` stores a copy of that
> pointer and dereferences it in several possible locations.
>
> Instead, the `AsyncExecutor` should store the `PID` of the actor,
> which can be safely used, even if the actor has already terminated;
> such as turning libprocess finalization.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/async.hpp 8565a52c6ba4008edb852e898b8f0b6d598a194f
>
> Diff: https://reviews.apache.org/r/55701/diff/
>
>
> Testing
> -------
>
> Applied the following change:
> ```
> diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
> index e035a4e..4c60ef9 100644
> --- a/src/launcher/executor.cpp
> +++ b/src/launcher/executor.cpp
> @@ -936,6 +930,8 @@ int main(int argc, char** argv)
>
> process::spawn(executor.get());
> process::wait(executor.get());
> + executor.reset();
>
> + process::finalize(true);
> return EXIT_SUCCESS;
> }
> ```
>
> And then ran:
> ```
> make check GTEST_FILTER="HTTPCommandExecutorTest.TerminateWithACK"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55701: Fixed unsafe usage of process pointer in
async.hpp.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55701/#review162279
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55701]
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 Jan. 19, 2017, 2:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55701/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 2:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `async(...)` helper spawns a libprocess actor to execute some
> lambda (asynchronously, of course). This actor is owned by the
> libprocess GC actor, but the `AsyncExecutor` stores a copy of that
> pointer and dereferences it in several possible locations.
>
> Instead, the `AsyncExecutor` should store the `PID` of the actor,
> which can be safely used, even if the actor has already terminated;
> such as turning libprocess finalization.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/async.hpp 8565a52c6ba4008edb852e898b8f0b6d598a194f
>
> Diff: https://reviews.apache.org/r/55701/diff/
>
>
> Testing
> -------
>
> Applied the following change:
> ```
> diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
> index e035a4e..4c60ef9 100644
> --- a/src/launcher/executor.cpp
> +++ b/src/launcher/executor.cpp
> @@ -936,6 +930,8 @@ int main(int argc, char** argv)
>
> process::spawn(executor.get());
> process::wait(executor.get());
> + executor.reset();
>
> + process::finalize(true);
> return EXIT_SUCCESS;
> }
> ```
>
> And then ran:
> ```
> make check GTEST_FILTER="HTTPCommandExecutorTest.TerminateWithACK"
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>