Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review100848
---


I'll get this in today :) Thanks Neil!

- Niklas Nielsen


On Sept. 24, 2015, 6:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Sept. 24, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review100509
---


Bad review!

Reviews applied: []

Error:
 No reviewers specified. Please find a reviewer by asking on JIRA or the 
mailing list.

- Mesos ReviewBot


On Sept. 24, 2015, 9:29 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Sept. 24, 2015, 9:29 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/
---

(Updated Sept. 24, 2015, 9:29 p.m.)


Review request for mesos.


Changes
---

Rebase.


Bugs: MESOS-3328
https://issues.apache.org/jira/browse/MESOS-3328


Repository: mesos


Description
---

The previous coding would try to shift a uint32_t value 32 bits to the left; per
C++ spec, this yields undefined behavior. On my machine, this resulted in
treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
wrong.

Spotted via ubsan: see MESOS-3328.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
1ad119d54820e97497b1773518875be25ddbf98a 
  3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 

Diff: https://reviews.apache.org/r/37903/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review99144
---


Patch looks great!

Reviews applied: [37903]

All tests passed.

- Mesos ReviewBot


On Sept. 15, 2015, 9:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Sept. 15, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/
---

(Updated Sept. 15, 2015, 9:10 p.m.)


Review request for mesos.


Bugs: MESOS-3328
https://issues.apache.org/jira/browse/MESOS-3328


Repository: mesos


Description
---

The previous coding would try to shift a uint32_t value 32 bits to the left; per
C++ spec, this yields undefined behavior. On my machine, this resulted in
treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
wrong.

Spotted via ubsan: see MESOS-3328.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
1ad119d54820e97497b1773518875be25ddbf98a 
  3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 

Diff: https://reviews.apache.org/r/37903/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Till Toenshoff]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review98592
---

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (line 399)


Please end this with a punctuation.


- Till Toenshoff


On Sept. 10, 2015, 5:07 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Sept. 10, 2015, 5:07 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review98373
---


Patch looks great!

Reviews applied: [37903]

All tests passed.

- Mesos ReviewBot


On Sept. 10, 2015, 5:07 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Sept. 10, 2015, 5:07 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/
---

(Updated Sept. 10, 2015, 5:07 a.m.)


Review request for mesos.


Bugs: MESOS-3328
https://issues.apache.org/jira/browse/MESOS-3328


Repository: mesos


Description
---

The previous coding would try to shift a uint32_t value 32 bits to the left; per
C++ spec, this yields undefined behavior. On my machine, this resulted in
treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
wrong.

Spotted via ubsan: see MESOS-3328.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
1ad119d54820e97497b1773518875be25ddbf98a 
  3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 

Diff: https://reviews.apache.org/r/37903/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

> On Sept. 9, 2015, 11:08 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 402
> > 
> >
> > An alternative could have been:
> > 
> > IP(prefix == 0 ? 0 : 0x << (32 - prefix))
> > 
> > Will let you decide what is easier to read.
> > I like it in terms of not writing IPNetwork twice

Good suggestion -- I wasn't entirely happy with the previous syntax, either. I 
ended up opting for being more explicit:

  // Avoid left-shifting by 32 bits when prefix is 0
  uint32_t mask = 0;
  if (prefix > 0) {
mask = 0x << (32 - prefix);
  }
  return IPNetwork(address, IP(mask));


- Neil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review98301
---


On Aug. 28, 2015, 8:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review98301
---

Ship it!


Minor nits


3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (line 401)


No need for the else block when you return at line 400



3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (line 402)


An alternative could have been:

IP(prefix == 0 ? 0 : 0x << (32 - prefix))

Will let you decide what is easier to read.
I like it in terms of not writing IPNetwork twice


- Niklas Nielsen


On Aug. 28, 2015, 1:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 1:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

> On Aug. 31, 2015, 10:28 p.m., Cong Wang wrote:
> > Or we can simply make 'prefix' unsigned.

Thanks for the review!

As far as I know, making "prefix" unsigned would not help. The code in question 
is: "0x << (32 - prefix)". 0x is an unsigned integer literal -- 
left-shifting a 32-bit integer (0x) by 32 bits is undefined, regardless 
of sign.


- Neil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review97170
---


On Aug. 28, 2015, 8:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Cong Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review97170
---

Ship it!


Or we can simply make 'prefix' unsigned.

- Cong Wang


On Aug. 28, 2015, 8:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/#review96934
---


Patch looks great!

Reviews applied: [37903]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 8:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37903/
---

Review request for mesos.


Bugs: MESOS-3328
https://issues.apache.org/jira/browse/MESOS-3328


Repository: mesos


Description
---

The previous coding would try to shift a uint32_t value 32 bits to the left; per
C++ spec, this yields undefined behavior. On my machine, this resulted in
treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
wrong.

Spotted via ubsan: see MESOS-3328.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
1ad119d54820e97497b1773518875be25ddbf98a 
  3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 

Diff: https://reviews.apache.org/r/37903/diff/


Testing
---

make check


Thanks,

Neil Conway