You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2020/01/03 01:38:05 UTC
Review Request 71947: Handled embedded null bytes in abstract domain
socket names.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Repository: mesos
Description
-------
Address handling code for unix domain sockets assumed that
strlen() could be used to compute the name of a unix domain
socket, but that fails for unnamed sockets or in the case
where an abstract domain socket contains embedded null bytes.
This patch adds a new `length` parameter to correctly handle
these special cases.
Diffs
-----
3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
Diff: https://reviews.apache.org/r/71947/diff/1/
Testing
-------
Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
Thanks,
Benno Evers
Re: Review Request 71947: Handled embedded null bytes in abstract
domain socket names.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/#review219207
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On Jan. 10, 2020, 2:37 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71947/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 2:37 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Address handling code for unix domain sockets assumed that
> strlen() could be used to compute the name of a unix domain
> socket, but that fails for unnamed sockets or in the case
> where an abstract domain socket contains embedded null bytes.
>
> This patch adds a new `length` parameter to correctly handle
> these special cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
> 3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
>
>
> Diff: https://reviews.apache.org/r/71947/diff/2/
>
>
> Testing
> -------
>
> Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71947: Handled embedded null bytes in abstract
domain socket names.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/
-----------------------------------------------------------
(Updated Jan. 10, 2020, 1:37 a.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Changes
-------
Cleaned up implementation to always return the correct length for unix addresses.
Repository: mesos
Description
-------
Address handling code for unix domain sockets assumed that
strlen() could be used to compute the name of a unix domain
socket, but that fails for unnamed sockets or in the case
where an abstract domain socket contains embedded null bytes.
This patch adds a new `length` parameter to correctly handle
these special cases.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
Diff: https://reviews.apache.org/r/71947/diff/2/
Changes: https://reviews.apache.org/r/71947/diff/1-2/
Testing
-------
Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
Thanks,
Benno Evers
Re: Review Request 71947: Handled embedded null bytes in abstract
domain socket names.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/#review219118
-----------------------------------------------------------
Patch looks great!
Reviews applied: [71947]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 3, 2020, 1:38 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71947/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2020, 1:38 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Address handling code for unix domain sockets assumed that
> strlen() could be used to compute the name of a unix domain
> socket, but that fails for unnamed sockets or in the case
> where an abstract domain socket contains embedded null bytes.
>
> This patch adds a new `length` parameter to correctly handle
> these special cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
> 3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
>
>
> Diff: https://reviews.apache.org/r/71947/diff/1/
>
>
> Testing
> -------
>
> Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71947: Handled embedded null bytes in abstract
domain socket names.
Posted by Benno Evers <be...@mesosphere.com>.
> On Jan. 6, 2020, 12:59 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Lines 238 (patched)
> > <https://reviews.apache.org/r/71947/diff/1/?file=2191675#file2191675line238>
> >
> > As discussed offline, let's add a `CHECK` here that `sun_path` is not empty.
As it turns out, I was mistaken in our offline discussion: a single null byte is a perfectly valid name for an abstract domain socket.
> On Jan. 6, 2020, 12:59 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Line 232 (original), 247 (patched)
> > <https://reviews.apache.org/r/71947/diff/1/?file=2191675#file2191675line247>
> >
> > For unnamed sockets we would now return an empty string here which seems leaky. What do you think about changing the return type of `Address::path` to e.g., `Option<string>` instead and returning a `None` in that case?
I've thought about it and an empty string actually uniquely identifies unnamed sockets. Pathname sockets always must have at least one character, and the difference between an empty string and an abstract domain socket whose name is a single null byte would be that the former has size 0, and the latter has size 1.
So I think as long as we document this correspondence, this should be fine.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/#review219127
-----------------------------------------------------------
On Jan. 10, 2020, 1:37 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71947/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Address handling code for unix domain sockets assumed that
> strlen() could be used to compute the name of a unix domain
> socket, but that fails for unnamed sockets or in the case
> where an abstract domain socket contains embedded null bytes.
>
> This patch adds a new `length` parameter to correctly handle
> these special cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
> 3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
>
>
> Diff: https://reviews.apache.org/r/71947/diff/2/
>
>
> Testing
> -------
>
> Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71947: Handled embedded null bytes in abstract
domain socket names.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Jan. 6, 2020, 1:59 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Line 232 (original), 247 (patched)
> > <https://reviews.apache.org/r/71947/diff/1/?file=2191675#file2191675line247>
> >
> > For unnamed sockets we would now return an empty string here which seems leaky. What do you think about changing the return type of `Address::path` to e.g., `Option<string>` instead and returning a `None` in that case?
>
> Benno Evers wrote:
> I've thought about it and an empty string actually uniquely identifies unnamed sockets. Pathname sockets always must have at least one character, and the difference between an empty string and an abstract domain socket whose name is a single null byte would be that the former has size 0, and the latter has size 1.
>
> So I think as long as we document this correspondence, this should be fine.
SG, dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/#review219127
-----------------------------------------------------------
On Jan. 10, 2020, 2:37 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71947/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 2:37 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Address handling code for unix domain sockets assumed that
> strlen() could be used to compute the name of a unix domain
> socket, but that fails for unnamed sockets or in the case
> where an abstract domain socket contains embedded null bytes.
>
> This patch adds a new `length` parameter to correctly handle
> these special cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
> 3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
>
>
> Diff: https://reviews.apache.org/r/71947/diff/2/
>
>
> Testing
> -------
>
> Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71947: Handled embedded null bytes in abstract
domain socket names.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71947/#review219127
-----------------------------------------------------------
3rdparty/libprocess/include/process/address.hpp
Lines 224 (patched)
<https://reviews.apache.org/r/71947/#comment307224>
Let's include `<cstddef>` for `offsetof`.
3rdparty/libprocess/include/process/address.hpp
Line 226 (original), 227-228 (patched)
<https://reviews.apache.org/r/71947/#comment307226>
This is not very clear to me. Can we give a very explicit example on how to obtain `_length`, or alternatively compute it ourself as suggested below?
3rdparty/libprocess/include/process/address.hpp
Lines 238 (patched)
<https://reviews.apache.org/r/71947/#comment307229>
As discussed offline, let's add a `CHECK` here that `sun_path` is not empty.
3rdparty/libprocess/include/process/address.hpp
Line 232 (original), 247 (patched)
<https://reviews.apache.org/r/71947/#comment307227>
For unnamed sockets we would now return an empty string here which seems leaky. What do you think about changing the return type of `Address::path` to e.g., `Option<string>` instead and returning a `None` in that case?
3rdparty/libprocess/include/process/address.hpp
Lines 275 (patched)
<https://reviews.apache.org/r/71947/#comment307228>
nit: Missing trailing dot.
3rdparty/libprocess/include/process/address.hpp
Lines 318 (patched)
<https://reviews.apache.org/r/71947/#comment307230>
Does it make sense to pass an `Option<socklen_t>` defaulted to `None` to make it clearer that `length` is only interesting in certain cases?
In any case we need to do add documentation on the semantics of the parameters.
- Benjamin Bannier
On Jan. 3, 2020, 2:38 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71947/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2020, 2:38 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Address handling code for unix domain sockets assumed that
> strlen() could be used to compute the name of a unix domain
> socket, but that fails for unnamed sockets or in the case
> where an abstract domain socket contains embedded null bytes.
>
> This patch adds a new `length` parameter to correctly handle
> these special cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp 749498056b52b916dfaf6c85f83ecc05e0d5406c
> 3rdparty/libprocess/include/process/network.hpp 8f48a4a78557309a9b1b00d7defb45eed454b077
>
>
> Diff: https://reviews.apache.org/r/71947/diff/1/
>
>
> Testing
> -------
>
> Ran existing unit tests and verified that the newly added `CHECK()` doesn't trigger.
>
>
> Thanks,
>
> Benno Evers
>
>