[jira] [Commented] (MESOS-3335) FlagsBase copy-ctor leads to dangling pointer.

["Michael Park (JIRA)" <ji...@apache.org>]

    [ 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 <be...@mesosphere.io>
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 <be...@mesosphere.io>
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 <be...@mesosphere.io>
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 <be...@mesosphere.io>
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 <be...@mesosphere.io>
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<MyFlags1, MyFlags2> 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<std::string> {
>     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)