[jira] [Commented] (MESOS-3335) FlagsBase copy-ctor leads to dangling pointer.
[
https://issues.apache.org/jira/browse/MESOS-3335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15585813#comment-15585813
]
Michael Park commented on MESOS-3335:
-
{noformat}
commit 92fb8e7ee2280b5b57840fd3d2f5f3e3814c
Author: Benjamin Bannier
Date: Tue Oct 18 03:29:54 2016 -0400
Removed the non-member pointer overloads of `FlagsBase::add`.
Review: https://reviews.apache.org/r/46825/
{noformat}
{noformat}
commit f441eb9adb8c1443e62e10d17ed4019b66391168
Author: Benjamin Bannier
Date: Tue Oct 18 04:48:10 2016 -0400
Fully qualified addresses of flag members in `add` calls in mesos.
While right now we can technically `add` variables to `Flags` classes
which are not members, the in order to have correct copy semantics for
`Flags` only member variables should be used.
Here we changed all instances to a full pointer-to-member syntax in
the current code.
Review: https://reviews.apache.org/r/46824/
{noformat}
{noformat}
commit dde5eee7b11be8df874571316e29a9a25ae59150
Author: Benjamin Bannier
Date: Tue Oct 18 03:29:39 2016 -0400
Fully qualified addresses of flag members in `add` calls in libprocess.
While right now we can technically `add` variables to `Flags` classes
which are not members, the in order to have correct copy semantics for
`Flags` only member variables should be used.
Here we changed all instances to a full pointer-to-member syntax in
the current code.
Review: https://reviews.apache.org/r/46823/
{noformat}
{noformat}
commit 5aeecca345f80d5d34c9a8c8b64d460bcd773e30
Author: Benjamin Bannier
Date: Tue Oct 18 03:29:14 2016 -0400
Fully qualified addresses of flag members in `add` calls in stout.
While right now we can technically `add` variables to `Flags` classes
which are not members, the in order to have correct copy semantics for
`Flags` only member variables should be used.
Here we changed all instances to a full pointer-to-member syntax in
the current code.
Review: https://reviews.apache.org/r/52390/
{noformat}
> FlagsBase copy-ctor leads to dangling pointer.
> --
>
> Key: MESOS-3335
> URL: https://issues.apache.org/jira/browse/MESOS-3335
> Project: Mesos
> Issue Type: Bug
>Reporter: Neil Conway
>Assignee: Benjamin Bannier
> Labels: mesosphere
> Attachments: lambda_capture_bug.cpp
>
>
> Per [#3328], ubsan detects the following problem:
> [ RUN ] FaultToleranceTest.ReregisterCompletedFrameworks
> /mesos/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:303:25:
> runtime error: load of value 33, which is not a valid value for type 'bool'
> I believe what is going on here is the following:
> * The test calls StartMaster(), which does MesosTest::CreateMasterFlags()
> * MesosTest::CreateMasterFlags() allocates a new master::Flags on the stack,
> which is subsequently copy-constructed back to StartMaster()
> * The FlagsBase constructor is:
> bq. {{FlagsBase() { add(&help, "help", "...", false); }}}
> where "help" is a member variable -- i.e., it is allocated on the stack in
> this case.
> * {{FlagsBase()::add}} captures {{&help}}, e.g.:
> {noformat}
> flag.stringify = [t1](const FlagsBase&) -> Option {
> return stringify(*t1);
> };}}
> {noformat}
> * The implicit copy constructor for FlagsBase is just going to copy the
> lambda above, i.e., the result of the copy constructor will have a lambda
> that points into MesosTest::CreateMasterFlags()'s stack frame, which is bad
> news.
> Not sure the right fix -- comments welcome. You could define a copy-ctor for
> FlagsBase that does something gross (basically remove the old help flag and
> define a new one that points into the target of the copy), but that seems,
> well, gross.
> Probably not a pressing-problem to fix -- AFAICS worst symptom is that we end
> up reading one byte from some random stack location when serving
> {{state.json}}, for example.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (MESOS-3335) FlagsBase copy-ctor leads to dangling pointer.
[
https://issues.apache.org/jira/browse/MESOS-3335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15581776#comment-15581776
]
Michael Park commented on MESOS-3335:
-
{noformat}
commit 82c520eb0d83710e3f0ed4514348f893c2b44455
Author: Benjamin Bannier
Date: Fri Oct 14 18:56:07 2016 -0400
Consistently used virtual inheritance for Flags classes in mesos.
In order for different `Flags` classes to be composable classes should
always use virtual inheritance.
Review: https://reviews.apache.org/r/49829/
{noformat}
{noformat}
commit bb903381fa8b98c1f16bd8af69c727d501ba99e9
Author: Benjamin Bannier
Date: Fri Oct 14 18:56:04 2016 -0400
Consistently used virtual inheritance for Flags classes in libprocess.
In order for different `Flags` classes to be composable classes should
always use virtual inheritance.
Review: https://reviews.apache.org/r/52387/
{noformat}
{noformat}
commit 16914ae87b999223e26d63415b56d5aca4bf8b2b
Author: Benjamin Bannier
Date: Fri Oct 14 18:56:02 2016 -0400
Consistently used virtual inheritance for Flags classes in stout.
In order for different `Flags` classes to be composable classes should
always use virtual inheritance.
Review: https://reviews.apache.org/r/49833/
{noformat}
{noformat}
commit 5d491eb77a9268feaec0587e79808e7805907317
Author: Benjamin Bannier
Date: Mon Oct 17 05:33:17 2016 -0400
Deleted potentially implicitly generated functions.
Here we explicitly disable rvalue constructors and assignment
operators for `flags::FlagsBase` since we plan for this class to be
used in virtual inheritance scenarios. Here e.g., constructing from
an rvalue will be problematic since we can potentially have multiple
lineages leading to the same base class, and could then potentially
use a moved from base object.
These functions would be implicitly generated only for C++14, but in
C++11 only the versions taking lvalue references should be. GCC seems
to create all of these even in C++11 mode so we need to explicitly
disable them.
Review: https://reviews.apache.org/r/52386/
{noformat}
{noformat}
commit da2aa2c14796b64b19002b86b3b6b9a443479ba8
Author: Benjamin Bannier
Date: Fri Oct 14 18:55:55 2016 -0400
Removed `flags::Flags` helper template.
The template `flags::Flags` allowed to compose flags classes on the
fly, e.g.,
flags::Flags flags;
which would create a class inheriting virtually from both `MyFlags1`
and `MyFlags2`.
This class was not used in the code.
Review: https://reviews.apache.org/r/52604/
{noformat}
> FlagsBase copy-ctor leads to dangling pointer.
> --
>
> Key: MESOS-3335
> URL: https://issues.apache.org/jira/browse/MESOS-3335
> Project: Mesos
> Issue Type: Bug
>Reporter: Neil Conway
>Assignee: Benjamin Bannier
> Labels: mesosphere
> Attachments: lambda_capture_bug.cpp
>
>
> Per [#3328], ubsan detects the following problem:
> [ RUN ] FaultToleranceTest.ReregisterCompletedFrameworks
> /mesos/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:303:25:
> runtime error: load of value 33, which is not a valid value for type 'bool'
> I believe what is going on here is the following:
> * The test calls StartMaster(), which does MesosTest::CreateMasterFlags()
> * MesosTest::CreateMasterFlags() allocates a new master::Flags on the stack,
> which is subsequently copy-constructed back to StartMaster()
> * The FlagsBase constructor is:
> bq. {{FlagsBase() { add(&help, "help", "...", false); }}}
> where "help" is a member variable -- i.e., it is allocated on the stack in
> this case.
> * {{FlagsBase()::add}} captures {{&help}}, e.g.:
> {noformat}
> flag.stringify = [t1](const FlagsBase&) -> Option {
> return stringify(*t1);
> };}}
> {noformat}
> * The implicit copy constructor for FlagsBase is just going to copy the
> lambda above, i.e., the result of the copy constructor will have a lambda
> that points into MesosTest::CreateMasterFlags()'s stack frame, which is bad
> news.
> Not sure the right fix -- comments welcome. You could define a copy-ctor for
> FlagsBase that does something gross (basically remove the old help flag and
> define a new one that points into the target of the copy), but that seems,
> well, gross.
> Probably not a pressing-problem to fix -- AFAICS worst symptom is that we end
> up reading one byte from some random stack location when serving
> {{state.json}}, for example.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (MESOS-3335) FlagsBase copy-ctor leads to dangling pointer.
[
https://issues.apache.org/jira/browse/MESOS-3335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15532718#comment-15532718
]
Benjamin Bannier commented on MESOS-3335:
-
Reviews:
https://reviews.apache.org/r/52388/
https://reviews.apache.org/r/46825/
https://reviews.apache.org/r/46823/
https://reviews.apache.org/r/52390/
https://reviews.apache.org/r/49829/
https://reviews.apache.org/r/52387/
https://reviews.apache.org/r/49833/
https://reviews.apache.org/r/52386/
https://reviews.apache.org/r/52385/
> FlagsBase copy-ctor leads to dangling pointer.
> --
>
> Key: MESOS-3335
> URL: https://issues.apache.org/jira/browse/MESOS-3335
> Project: Mesos
> Issue Type: Bug
>Reporter: Neil Conway
>Assignee: Benjamin Bannier
> Labels: mesosphere
> Attachments: lambda_capture_bug.cpp
>
>
> Per [#3328], ubsan detects the following problem:
> [ RUN ] FaultToleranceTest.ReregisterCompletedFrameworks
> /mesos/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:303:25:
> runtime error: load of value 33, which is not a valid value for type 'bool'
> I believe what is going on here is the following:
> * The test calls StartMaster(), which does MesosTest::CreateMasterFlags()
> * MesosTest::CreateMasterFlags() allocates a new master::Flags on the stack,
> which is subsequently copy-constructed back to StartMaster()
> * The FlagsBase constructor is:
> bq. {{FlagsBase() { add(&help, "help", "...", false); }}}
> where "help" is a member variable -- i.e., it is allocated on the stack in
> this case.
> * {{FlagsBase()::add}} captures {{&help}}, e.g.:
> {noformat}
> flag.stringify = [t1](const FlagsBase&) -> Option {
> return stringify(*t1);
> };}}
> {noformat}
> * The implicit copy constructor for FlagsBase is just going to copy the
> lambda above, i.e., the result of the copy constructor will have a lambda
> that points into MesosTest::CreateMasterFlags()'s stack frame, which is bad
> news.
> Not sure the right fix -- comments welcome. You could define a copy-ctor for
> FlagsBase that does something gross (basically remove the old help flag and
> define a new one that points into the target of the copy), but that seems,
> well, gross.
> Probably not a pressing-problem to fix -- AFAICS worst symptom is that we end
> up reading one byte from some random stack location when serving
> {{state.json}}, for example.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (MESOS-3335) FlagsBase copy-ctor leads to dangling pointer.
[
https://issues.apache.org/jira/browse/MESOS-3335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15438155#comment-15438155
]
Michael Park commented on MESOS-3335:
-
{noformat}
commit f3181999fd03c99078994855687749839eb5365f
Author: Benjamin Bannier
Date: Thu Aug 25 13:53:15 2016 -0700
Avoided slicing of flags from `subprocess` in mesos.
While `FlagsBase` internally stores just maps of names and
flags, the functions stored in a `Flag` rely on the original type of
the `Flags` containing them (e.g., we perform dynamic casts to detect
their types).
Since `Option` stores `T` as a value (i.e., it cannot contain
reference types) any interface taking an `Option` cannot rely on
polymorphic behavior of `T`. To make use of polymorphism we should
instead store e.g., a pointer type to avoid slicing.
Here we change `Flags` arguments of `subprocess` to allow preserving
the original type so `Flag` functions can work reliably; we do this by
passing a non-owning pointer to a `Flags` so we do not restrict copying.
Review: https://reviews.apache.org/r/46822/
{noformat}
{noformat}
commit d05625fd7dc8cdf8c0007f2d9ddd2d739cfd9f5d
Author: Benjamin Bannier
Date: Thu Aug 25 13:53:10 2016 -0700
Avoided slicing of flags from `subprocess` in libprocess.
While `FlagsBase` internally stores just maps of names and
flags, the functions stored in a `Flag` rely on the original type of
the `Flags` containing them (e.g., we perform dynamic casts to detect
their types).
Since `Option` stores `T` as a value (i.e., it cannot contain
reference types) any interface taking an `Option` cannot rely on
polymorphic behavior of `T`. To make use of polymorphism we should
instead store e.g., a pointer type to avoid slicing.
Here we change `Flags` arguments of `subprocess` to allow preserving
the original type so `Flag` functions can work reliably; we do this by
passing a non-owning pointer to a `Flags` so we do not restrict copying.
Review: https://reviews.apache.org/r/46821/
{noformat}
> FlagsBase copy-ctor leads to dangling pointer.
> --
>
> Key: MESOS-3335
> URL: https://issues.apache.org/jira/browse/MESOS-3335
> Project: Mesos
> Issue Type: Bug
>Reporter: Neil Conway
>Assignee: Benjamin Bannier
> Labels: mesosphere
> Attachments: lambda_capture_bug.cpp
>
>
> Per [#3328], ubsan detects the following problem:
> [ RUN ] FaultToleranceTest.ReregisterCompletedFrameworks
> /mesos/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:303:25:
> runtime error: load of value 33, which is not a valid value for type 'bool'
> I believe what is going on here is the following:
> * The test calls StartMaster(), which does MesosTest::CreateMasterFlags()
> * MesosTest::CreateMasterFlags() allocates a new master::Flags on the stack,
> which is subsequently copy-constructed back to StartMaster()
> * The FlagsBase constructor is:
> bq. {{FlagsBase() { add(&help, "help", "...", false); }}}
> where "help" is a member variable -- i.e., it is allocated on the stack in
> this case.
> * {{FlagsBase()::add}} captures {{&help}}, e.g.:
> {noformat}
> flag.stringify = [t1](const FlagsBase&) -> Option {
> return stringify(*t1);
> };}}
> {noformat}
> * The implicit copy constructor for FlagsBase is just going to copy the
> lambda above, i.e., the result of the copy constructor will have a lambda
> that points into MesosTest::CreateMasterFlags()'s stack frame, which is bad
> news.
> Not sure the right fix -- comments welcome. You could define a copy-ctor for
> FlagsBase that does something gross (basically remove the old help flag and
> define a new one that points into the target of the copy), but that seems,
> well, gross.
> Probably not a pressing-problem to fix -- AFAICS worst symptom is that we end
> up reading one byte from some random stack location when serving
> {{state.json}}, for example.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (MESOS-3335) FlagsBase copy-ctor leads to dangling pointer
[
https://issues.apache.org/jira/browse/MESOS-3335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15245869#comment-15245869
]
Benjamin Bannier commented on MESOS-3335:
-
This appears a bigger problem than just possibly mutilating some help string.
Above call to {{add}} uses a pointer to memory not necessarily associated to
the {{Flags}} object for both reading and writing, and like you show even in
situations where the scope holding on to that memory has already been cleaned
up. We do use this {{add}} overload (there are actually two of them) to add a
large portion of existing {{Flag}} values.
This being undefined behavior we cannot rely on this error just messing up
things locally; in fact e.g., an optimized test built built with clang-3.9.0
will immediately segfault because of this.
I believe a possible fix would be to enforce that we only pass pointers to
member variables and add the usage sites get the actual values via the current
{{Flags}} object; this is already used partially to implement {{add}} support
of {{Flag}} values from derived {{Flags}}.
> FlagsBase copy-ctor leads to dangling pointer
> -
>
> Key: MESOS-3335
> URL: https://issues.apache.org/jira/browse/MESOS-3335
> Project: Mesos
> Issue Type: Bug
>Reporter: Neil Conway
>Assignee: Benjamin Bannier
>Priority: Minor
> Attachments: lambda_capture_bug.cpp
>
>
> Per [#3328], ubsan detects the following problem:
> [ RUN ] FaultToleranceTest.ReregisterCompletedFrameworks
> /mesos/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:303:25:
> runtime error: load of value 33, which is not a valid value for type 'bool'
> I believe what is going on here is the following:
> * The test calls StartMaster(), which does MesosTest::CreateMasterFlags()
> * MesosTest::CreateMasterFlags() allocates a new master::Flags on the stack,
> which is subsequently copy-constructed back to StartMaster()
> * The FlagsBase constructor is:
> bq. {{FlagsBase() { add(&help, "help", "...", false); }}}
> where "help" is a member variable -- i.e., it is allocated on the stack in
> this case.
> * {{FlagsBase()::add}} captures {{&help}}, e.g.:
> {noformat}
> flag.stringify = [t1](const FlagsBase&) -> Option {
> return stringify(*t1);
> };}}
> {noformat}
> * The implicit copy constructor for FlagsBase is just going to copy the
> lambda above, i.e., the result of the copy constructor will have a lambda
> that points into MesosTest::CreateMasterFlags()'s stack frame, which is bad
> news.
> Not sure the right fix -- comments welcome. You could define a copy-ctor for
> FlagsBase that does something gross (basically remove the old help flag and
> define a new one that points into the target of the copy), but that seems,
> well, gross.
> Probably not a pressing-problem to fix -- AFAICS worst symptom is that we end
> up reading one byte from some random stack location when serving
> {{state.json}}, for example.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
