You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Mahler <be...@gmail.com> on 2015/12/04 04:20:32 UTC
Re: mesos git commit: Fixed pointer alignment error in IP::create().
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, <mp...@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 <ne...@gmail.com>
> Authored: Thu Dec 3 03:13:22 2015 -0500
> Committer: Michael Park <mp...@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));
> + }
> + }
> }
>
>
>
>
Re: mesos git commit: Fixed pointer alignment error in IP::create().
Posted by Benjamin Mahler <be...@gmail.com>.
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 <be...@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 <mc...@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, <mp...@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 <ne...@gmail.com>
>> > > Authored: Thu Dec 3 03:13:22 2015 -0500
>> > > Committer: Michael Park <mp...@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));
>> > > + }
>> > > + }
>> > > }
>> > >
>> > >
>> > >
>> > >
>> >
>>
>
>
Re: mesos git commit: Fixed pointer alignment error in IP::create().
Posted by Benjamin Mahler <be...@gmail.com>.
(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 <mc...@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, <mp...@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 <ne...@gmail.com>
> > > Authored: Thu Dec 3 03:13:22 2015 -0500
> > > Committer: Michael Park <mp...@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));
> > > + }
> > > + }
> > > }
> > >
> > >
> > >
> > >
> >
>
Re: mesos git commit: Fixed pointer alignment error in IP::create().
Posted by Michael Park <mc...@gmail.com>.
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 <be...@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, <mp...@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 <ne...@gmail.com>
> > Authored: Thu Dec 3 03:13:22 2015 -0500
> > Committer: Michael Park <mp...@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));
> > + }
> > + }
> > }
> >
> >
> >
> >
>