Re: Review Request 71947: Handled embedded null bytes in abstract domain socket names.

[Benjamin Bannier]

---
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.

[Benjamin Bannier]

> On Jan. 6, 2020, 1:59 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Line 232 (original), 247 (patched)
> > 
> >
> > 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` 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.

[Benno Evers]

> On Jan. 6, 2020, 12:59 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Lines 238 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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` 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.

[Benno Evers]

---
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.

[Benjamin Bannier]

---
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)


Let's include `` for `offsetof`.



3rdparty/libprocess/include/process/address.hpp
Line 226 (original), 227-228 (patched)


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)


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)


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` instead and returning a `None` in that case?



3rdparty/libprocess/include/process/address.hpp
Lines 275 (patched)


nit: Missing trailing dot.



3rdparty/libprocess/include/process/address.hpp
Lines 318 (patched)


Does it make sense to pass an `Option` 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
> 
>

Re: Review Request 71947: Handled embedded null bytes in abstract domain socket names.

[Mesos Reviewbot]

---
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
> 
>

Review Request 71947: Handled embedded null bytes in abstract domain socket names.

[Benno Evers]

---
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