Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mesos.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAFp_NisyhhMgXV-4SqVOp4PYA5NDairXKSukFZSywUfoLzVydg@mail.gmail.com>
References: <544a236de9d64657b195122b7f68957b@git.apache.org>
 <CAFp_Nis8oGp4BVqBJOdhD=HhOXxW6nUc9j6PKE1AgCxHQKDLfg@mail.gmail.com>
 <CAL3vCQpXsgkvKLj7nYwmg6ka2ihaqtKn+dLXijs7M=eTB-AXwg@mail.gmail.com>
 <CAFp_NisyhhMgXV-4SqVOp4PYA5NDairXKSukFZSywUfoLzVydg@mail.gmail.com>
From: Benjamin Mahler <benjamin.mahler@gmail.com>
Date: Fri, 4 Dec 2015 12:04:06 -0800
Message-ID: 
 <CAFp_Niv2=uuGdFE3O6Npkeh726Vv+39sfQY6j+pcK3LB09kT2A@mail.gmail.com>
Subject: Re: mesos git commit: Fixed pointer alignment error in IP::create().
To: dev <dev@mesos.apache.org>
Cc: Neil Conway <neil@mesosphere.io>, Michael Park <mpark@mesosphere.io>,
 commits@mesos.apache.org
Content-Type: multipart/alternative; boundary=089e01160600b0fae0052618034e

--089e01160600b0fae0052618034e
Content-Type: text/plain; charset=UTF-8

Sorry, the first example from protobuf_utils.cpp isn't so bad, although
we're inconsistent with how we name iterators. Also, it would be great to
use foreach with a reversed adaptor rather than manual reverse iteration.

On Fri, Dec 4, 2015 at 12:01 PM, Benjamin Mahler <benjamin.mahler@gmail.com>
wrote:

> (Hm.. were you using bold formatting here? Apache's mail servers will
> strip it and change it into *asterisks*.)
>
> Yes, in general we want to be conservative with auto, and a lot of
> additions got through review that should not have. For example:
>
> src/common/protobuf_utils.cpp:  for (auto status =
> task.statuses().rbegin();
> src/local/local.cpp:    auto authorizerNames =
> strings::split(flags.authorizers, ",");
> src/master/http.cpp:  auto ids =
> ::protobuf::parse<RepeatedPtrField<MachineID>>(jsonIds.get());
> src/slave/containerizer/mesos/provisioner/docker/token_manager.cpp:
>  const auto padding = segment.length() % 4;
> src/tests/fault_tolerance_tests.cpp:  auto capabilityType =
> FrameworkInfo::Capability::REVOCABLE_RESOURCES;
>
> These definitely do not fit with our style guidelines around improving
> readability. The potential traps you've described are even more reason to
> be conservative with auto usage in our project.
>
> In many of these cases, the type is not obvious from the right-hand-side.
> If the person reading the code has to think in order to figure out what
> 'auto' is, we shouldn't be using it. My big concern here is that we need to
> make it as easy as possible to read and understand the code.
>
> Also, if the type is not onerous, why be aggressive with 'auto'?
>
> On Fri, Dec 4, 2015 at 1:10 AM, Michael Park <mcypark@gmail.com> wrote:
>
>> Ah, I didn't realize the current wording of the *auto* use cases were that
>> strict. But let me explain, and see what you and others think.
>>
>> First off, we already have uses of *const auto* and *const **auto&* in the
>> codebase. From my reading, it seems that you feel the same way about them
>> as you do towards *auto** (correct me if I'm reading wrong). I think
>>
>> The deduction rules for *auto* are equivalent to template argument
>> deduction rules. One must know and understand that fact in order to use
>> *auto* correctly. Given *auto x = expr;* one may "intuitively" think that
>> *auto *simply takes on the exact type of the right hand side, which is not
>> what happens (we have *decltype(auto)* for that). So I don't agree that
>> *auto&* and *auto** are any less intuitive than *auto*.
>>
>> IMO, it follows that the guideline for the qualifiers surrounding *auto*
>> is
>> equivalent to the guideline for qualifiers surrounding a template
>> parameter.
>>
>> *  template <typename T> void F(T x);*  *F(expr); *an argument of
>> arbitrary
>> type by value, *auto x = expr;*
>> *  template <typename T> void F(const T& x);  F(expr);* an argument of
>> arbitrary type by const-ref *const auto& x = expr;*
>> *  template <typename T> void F(const T* x);  F(expr);* an argument of
>> arbitrary type, const-pointer *const auto* x = expr;*
>> ...
>>
>> In the case being considered, we could have simply used *auto* because a
>> decaying a pointer has no effect (I don't think this is obvious).
>>
>> *  auto ptr = static_cast<const T*>(&t);*
>>   // *const T** decayed is *const T**, so *ptr* is *const T** and we're
>> ok.
>>
>> However, consider how a seemingly innocent modification can become
>> incorrect with an assumption that *auto* simply takes the exact type of
>> the
>> right hand side:
>>
>>   *auto ref = static_cast<const T&>(t);*
>>   // incorrect assumption: type of *ref* is *const T&*
>>   *const T* ptr = &ref;*
>>   // incorrect assumption: *ptr == &t*
>>
>> We actually need: *const auto& ref = static_cast<const T&>(t);* in order
>> for *ptr == &t* to be true. With that in mind, I would prefer to write the
>> above code as:
>>
>>   *const auto* ptr = static_cast<const T*>(&t);*
>>
>> and the innocent-looking modification as:
>>
>>   *const auto& ref = static_cast<const T&>(t);*
>> *  const T* ptr = &ref;*
>>
>> Thanks,
>>
>> MPark.
>>
>> On Thu, Dec 3, 2015 at 10:20 PM Benjamin Mahler <
>> benjamin.mahler@gmail.com>
>> wrote:
>>
>> > Why did you guys choose to use 'auto*' here? These are the first
>> instances
>> > in the code base, can we remove them? Let's consider 'auto&' and 'auto*'
>> > and update the style guide first, IMHO 'auto*' and 'auto&' are less
>> > intuitive, so we may want to be more conservative about them in general.
>> >
>> > On Thu, Dec 3, 2015 at 12:39 AM, <mpark@apache.org> wrote:
>> >
>> > > Repository: mesos
>> > > Updated Branches:
>> > >   refs/heads/master 0a82c2b12 -> 49d3cb112
>> > >
>> > >
>> > > Fixed pointer alignment error in IP::create().
>> > >
>> > > The previous code took the address of a `struct sockaddr`, and then
>> cast
>> > > the
>> > > resulting pointer to `struct sockaddr_storage *`. The alignment
>> > > requirements
>> > > for `struct sockaddr_storage *` are more strict than for `struct
>> sockaddr
>> > > *`,
>> > > and hence this code produced undefined behavior per ubsan in GCC 5.2.
>> > >
>> > > Drive-by Fix: Updated the code to use `reinterpret_cast` rather than a
>> > > C-style
>> > > cast, and to preserve constness.
>> > >
>> > > Review: https://reviews.apache.org/r/40435
>> > >
>> > >
>> > > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> > > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/49d3cb11
>> > > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/49d3cb11
>> > > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/49d3cb11
>> > >
>> > > Branch: refs/heads/master
>> > > Commit: 49d3cb1125cfcc8644e7f37b8d8654729aecd657
>> > > Parents: 0a82c2b
>> > > Author: Neil Conway <neil.conway@gmail.com>
>> > > Authored: Thu Dec 3 03:13:22 2015 -0500
>> > > Committer: Michael Park <mpark@apache.org>
>> > > Committed: Thu Dec 3 03:21:50 2015 -0500
>> > >
>> > > ----------------------------------------------------------------------
>> > >  .../3rdparty/stout/include/stout/ip.hpp         | 39
>> > ++++++++++++++------
>> > >  1 file changed, 27 insertions(+), 12 deletions(-)
>> > > ----------------------------------------------------------------------
>> > >
>> > >
>> > >
>> > >
>> >
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/49d3cb11/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp
>> > > ----------------------------------------------------------------------
>> > > diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp
>> > > b/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp
>> > > index 3e506e1..1d34d4e 100644
>> > > --- a/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp
>> > > +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp
>> > > @@ -211,23 +211,38 @@ inline Try<IP> IP::parse(const std::string&
>> value,
>> > > int family)
>> > >
>> > >  inline Try<IP> IP::create(const struct sockaddr_storage& _storage)
>> > >  {
>> > > -  switch (_storage.ss_family) {
>> > > -    case AF_INET: {
>> > > -      struct sockaddr_in addr = *((struct sockaddr_in*) &_storage);
>> > > -      return IP(addr.sin_addr);
>> > > -    }
>> > > -    default: {
>> > > -      return Error(
>> > > -          "Unsupported family type: " +
>> > > -          stringify(_storage.ss_family));
>> > > -    }
>> > > -  }
>> > > +  // According to POSIX: (IEEE Std 1003.1, 2004)
>> > > +  //
>> > > +  // (1) `sockaddr_storage` is "aligned at an appropriate boundary so
>> > that
>> > > +  // pointers to it can be cast as pointers to protocol-specific
>> address
>> > > +  // structures and used to access the fields of those structures
>> > without
>> > > +  // alignment problems."
>> > > +  //
>> > > +  // (2) "When a `sockaddr_storage` structure is cast as a `sockaddr`
>> > > +  // structure, the `ss_family` field of the `sockaddr_storage`
>> > structure
>> > > +  // shall map onto the `sa_family` field of the `sockaddr`
>> structure."
>> > > +  //
>> > > +  // Therefore, casting from `const sockaddr_storage*` to `const
>> > > sockaddr*`
>> > > +  // (then subsequently dereferencing the `const sockaddr*`) should
>> be
>> > > safe.
>> > > +  // Note that casting in the reverse direction (`const sockaddr*` to
>> > > +  // `const sockaddr_storage*`) would NOT be safe, since the former
>> > might
>> > > +  // not be aligned appropriately.
>> > > +  const auto* addr = reinterpret_cast<const struct
>> > sockaddr*>(&_storage);
>> > > +  return create(*addr);
>> > >  }
>> > >
>> > >
>> > >  inline Try<IP> IP::create(const struct sockaddr& _storage)
>> > >  {
>> > > -  return create(*((struct sockaddr_storage*) &_storage));
>> > > +  switch (_storage.sa_family) {
>> > > +    case AF_INET: {
>> > > +      const auto* addr = reinterpret_cast<const struct
>> > > sockaddr_in*>(&_storage);
>> > > +      return IP(addr->sin_addr);
>> > > +    }
>> > > +    default: {
>> > > +      return Error("Unsupported family type: " +
>> > > stringify(_storage.sa_family));
>> > > +    }
>> > > +  }
>> > >  }
>> > >
>> > >
>> > >
>> > >
>> >
>>
>
>

--089e01160600b0fae0052618034e--