Here is another problematic use case that I ran into a few weeks back:

Running the following ends up enteringa  recursive loop and blowing up the
stack:

```      container.id.mutable_parent()->CopyFrom(container.id);
```

If you look at the body of `MergeFrom()`, which is called from
`CopyFrom()`, you can see why:

```
void ContainerID::MergeFrom(const ContainerID& from) {
  GOOGLE_CHECK_NE(&from, this);
  if (from._has_bits_[0 / 32] & (0xffu << (0 % 32))) {
    if (from.has_value()) {
      set_value(from.value());
    }
    if (from.has_parent()) {
      mutable_parent()->::mesos::ContainerID::MergeFrom(from.parent());
    }
  }
  mutable_unknown_fields()->MergeFrom(from.unknown_fields());
}
```

When we call `CopyFrom()` we pass it the same object who’s parent we are
trying to modify.  However, once we make the original `mutable_parent()`
call, a parent has been created (albeit an empty one) on the original
object. This allows the `from.has_parent()` call in `MergeFrom()` to
succeed. From there we enter a recursive loop in calls to
`MergeFrom()`since we always operate on the same object.

On Mon, Sep 19, 2016 at 1:33 AM haosdent <haosd...@gmail.com> wrote:

> From mesos.pb.h/mesos.pb.cc
>
> ```
>   inline ContainerID& operator=(const ContainerID& from) {
>     CopyFrom(from);
>     return *this;
>   }
>
> void ContainerID::CopyFrom(const ::google::protobuf::Message& from) {
>   if (&from == this) return;
>   Clear();
>   MergeFrom(from);
> }
> ```
>
> On Mon, Sep 19, 2016 at 4:32 PM, haosdent <haosd...@gmail.com> wrote:
>
> > @Neil, when
> >
> > ```
> > rootContainerId = rootContainerId.parent();
> > ```
> >
> > protobuf would try to call `ContaienrID::Clear()` first and then perform
> > `ContainerID::CopyFrom`. Because the parent has been broken after
> > `ContainerID::Clear()`, so the `ContainerID::CopyFrom` would get an empty
> > value. I think this is not a bug of protobuf and we should avoid using
> >
> > ```
> > Message = Message.xx();
> > ```
> >
> > On Mon, Sep 19, 2016 at 3:42 PM, Neil Conway <neil.con...@gmail.com>
> > wrote:
> >
> >> Hi Jie,
> >>
> >> Do you have more details on what exactly the problem is here? If
> >> protobuf is unable to copy/merge nested messages in general, that
> >> seems like something that might crop up elsewhere.
> >>
> >> Perhaps we can (a) file a JIRA (ideally with a self-contained
> >> test-case), and/or (c) report the problem to upstream?
> >>
> >> Neil
> >>
> >> ---------- Forwarded message ----------
> >> From:  <ji...@apache.org>
> >> Date: Sat, Sep 17, 2016 at 11:27 PM
> >> Subject: mesos git commit: Fixed a bug in getRootContainerId due to
> >> protobuf copying issue.
> >> To: comm...@mesos.apache.org
> >>
> >>
> >> Repository: mesos
> >> Updated Branches:
> >>   refs/heads/master a4fd86bce -> be81a924a
> >>
> >>
> >> Fixed a bug in getRootContainerId due to protobuf copying issue.
> >>
> >> It looks like protobuf is not so great dealing with nesting messages
> >> when doing merge or copy. This patch uses an extra copy to bypass that
> >> issue in the protobuf.
> >>
> >> Review: https://reviews.apache.org/r/51992
> >>
> >>
> >> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> >> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/be81a924
> >> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/be81a924
> >> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/be81a924
> >>
> >> Branch: refs/heads/master
> >> Commit: be81a924a9e9414ec98f8c9a87a5391dad865146
> >> Parents: a4fd86b
> >> Author: Jie Yu <yujie....@gmail.com>
> >> Authored: Sat Sep 17 14:22:31 2016 -0700
> >> Committer: Jie Yu <yujie....@gmail.com>
> >> Committed: Sat Sep 17 14:25:33 2016 -0700
> >>
> >> ----------------------------------------------------------------------
> >>  src/slave/containerizer/mesos/utils.hpp | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> ----------------------------------------------------------------------
> >>
> >>
> >> http://git-wip-us.apache.org/repos/asf/mesos/blob/be81a924/s
> >> rc/slave/containerizer/mesos/utils.hpp
> >> ----------------------------------------------------------------------
> >> diff --git a/src/slave/containerizer/mesos/utils.hpp
> >> b/src/slave/containerizer/mesos/utils.hpp
> >> index 2bb55c1..178ebf3 100644
> >> --- a/src/slave/containerizer/mesos/utils.hpp
> >> +++ b/src/slave/containerizer/mesos/utils.hpp
> >> @@ -27,7 +27,12 @@ static ContainerID getRootContainerId(const
> >> ContainerID& containerId)
> >>  {
> >>    ContainerID rootContainerId = containerId;
> >>    while (rootContainerId.has_parent()) {
> >> -    rootContainerId = rootContainerId.parent();
> >> +    // NOTE: Looks like protobuf does not handle copying well when
> >> +    // nesting message is involved, because the source and the target
> >> +    // point to the same object. Therefore, we create a temporary
> >> +    // variable and use an extra copy here.
> >> +    ContainerID id = rootContainerId.parent();
> >> +    rootContainerId = id;
> >>    }
> >>
> >>    return rootContainerId;
> >>
> >
> >
> >
> > --
> > Best Regards,
> > Haosdent Huang
> >
>
>
>
> --
> Best Regards,
> Haosdent Huang
>

Reply via email to