Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-18 Thread Michael Park 


> On Nov. 17, 2016, 1:01 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/decoder.hpp, lines 253-254
> > 
> >
> > Do you happen to know what the request body length has to do with 
> > `CHAR_MAX` in the first place...?
> 
> Aaron Wood wrote:
> I'm not 100% clear on this but my guess is that it's from a negotiated 
> max body size between the server and clients within Mesos...?
> 
> James Peach wrote:
> AFAICT this is assigning the `Content-Length` using `basic_string& 
> operator=( CharT ch );`, which is entirely broken.
> 
> Aaron Wood wrote:
> Ignore my comment. This looks like a bug since it'll cause requests 
> coming in from clients using gzip to be greatly truncated.

We'll let Anand and Joseph take over this issue as a separate task.


- Michael


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


On Nov. 18, 2016, 8:24 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Nov. 18, 2016, 8:24 a.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 76dca0b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-18 Thread Aaron Wood 


> On Nov. 17, 2016, 9:01 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/decoder.hpp, lines 253-254
> > 
> >
> > Do you happen to know what the request body length has to do with 
> > `CHAR_MAX` in the first place...?
> 
> Aaron Wood wrote:
> I'm not 100% clear on this but my guess is that it's from a negotiated 
> max body size between the server and clients within Mesos...?
> 
> James Peach wrote:
> AFAICT this is assigning the `Content-Length` using `basic_string& 
> operator=( CharT ch );`, which is entirely broken.

Ignore my comment. This looks like a bug since it'll cause requests coming in 
from clients using gzip to be greatly truncated.


- Aaron


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


On Nov. 18, 2016, 4:24 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Nov. 18, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 76dca0b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-18 Thread James Peach 


> On Nov. 17, 2016, 9:01 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/decoder.hpp, lines 253-254
> > 
> >
> > Do you happen to know what the request body length has to do with 
> > `CHAR_MAX` in the first place...?
> 
> Aaron Wood wrote:
> I'm not 100% clear on this but my guess is that it's from a negotiated 
> max body size between the server and clients within Mesos...?

AFAICT this is assigning the `Content-Length` using `basic_string& operator=( 
CharT ch );`, which is entirely broken.


- James


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


On Nov. 18, 2016, 4:24 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Nov. 18, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 76dca0b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-18 Thread Aaron Wood 

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

(Updated Nov. 18, 2016, 4:24 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments and made a few small fixes.


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


Repository: mesos


Description
---

The hardening flags produced many new sign comparison errors in libprocess that 
need to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 76dca0b 
  3rdparty/libprocess/src/encoder.hpp 005d1cc 
  3rdparty/libprocess/src/process.cpp ab2b5a9 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
  3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
  3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran `make && make check && make bench`.


Thanks,

Aaron Wood

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-18 Thread Aaron Wood 


> On Nov. 17, 2016, 9:01 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/decoder.hpp, line 21
> > 
> >
> > `#include `

My fault, I have no idea why I decided to add a .h at the time :)


- Aaron


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


On Nov. 7, 2016, 4:45 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Nov. 7, 2016, 4:45 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 76dca0b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-18 Thread Aaron Wood 


> On Nov. 17, 2016, 9:01 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/decoder.hpp, lines 253-254
> > 
> >
> > Do you happen to know what the request body length has to do with 
> > `CHAR_MAX` in the first place...?

I'm not 100% clear on this but my guess is that it's from a negotiated max body 
size between the server and clients within Mesos...?


- Aaron


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


On Nov. 7, 2016, 4:45 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Nov. 7, 2016, 4:45 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 76dca0b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-07 Thread Aaron Wood 

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

(Updated Nov. 7, 2016, 4:45 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The hardening flags produced many new sign comparison errors in libprocess that 
need to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 76dca0b 
  3rdparty/libprocess/src/encoder.hpp 005d1cc 
  3rdparty/libprocess/src/process.cpp ab2b5a9 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
  3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
  3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran `make && make check && make bench`.


Thanks,

Aaron Wood

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-03 Thread Benjamin Bannier 


> On Nov. 2, 2016, 11:52 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/encoder.hpp, line 290
> > 
> >
> > I think using an `off_t` for a size is semantically incorrect; I'd stay 
> > with `size_t`. This requires adjusting the usage above.
> 
> Aaron Wood wrote:
> James Peach and I had a discussion about this and thought that `off_t` is 
> more correct for representing file sizes. Why would you prefer to stick with 
> `size_t`?
> 
> James Peach wrote:
> `FileEncoder` is taking the size of a file, which is `off_t`. The only 
> use of this class is `FileEncoder(socket, fd, s.st_size)`, which is 
> implicitly converting the `off_t` to `size_t`. If I had realized that at the 
> time, I would have asked for the `FileEncoder` constructor to also take an 
> `off_t` :)

I understand, dropping.


> On Nov. 2, 2016, 11:52 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 3735
> > 
> >
> > While `vector::size_type` is the correct type here, we 
> > typically just use `size_t`.
> 
> Aaron Wood wrote:
> Wouldn't `container::size_type` be more portable? `size_t` could vary on 
> the platform where `size_type` is container dependent.

No, I believe `size_t` is safe here. If I am reading the standard correctly, 
`container::size_type` represents "a type that can represent the size of the 
largest object in the allocation model" (`size_type` comes out of the allocator 
used for the container), and the possible range for `size_t` might be even 
bigger, but never smaller ("The type `size_t` is an implementation-defined 
unsigned integer type that is large enough to contain the size in bytes of any 
object"). This means that using `size_t` as iteration variable when iterating 
over a container should never cause truncation.

Also note that for `std::allocator` (which we use for our containers) 
`size_type` is a `typedef` for `size_t`, and in C++17 `allocator::size_type` is 
deprecated altogether.


- Benjamin


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


On Oct. 27, 2016, 6:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 27, 2016, 6:51 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-02 Thread James Peach 


> On Nov. 2, 2016, 10:52 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/encoder.hpp, line 290
> > 
> >
> > I think using an `off_t` for a size is semantically incorrect; I'd stay 
> > with `size_t`. This requires adjusting the usage above.
> 
> Aaron Wood wrote:
> James Peach and I had a discussion about this and thought that `off_t` is 
> more correct for representing file sizes. Why would you prefer to stick with 
> `size_t`?

`FileEncoder` is taking the size of a file, which is `off_t`. The only use of 
this class is `FileEncoder(socket, fd, s.st_size)`, which is implicitly 
converting the `off_t` to `size_t`. If I had realized that at the time, I would 
have asked for the `FileEncoder` constructor to also take an `off_t` :)


- James


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


On Oct. 27, 2016, 4:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 27, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-02 Thread Aaron Wood 


> On Nov. 2, 2016, 10:52 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/encoder.hpp, line 290
> > 
> >
> > I think using an `off_t` for a size is semantically incorrect; I'd stay 
> > with `size_t`. This requires adjusting the usage above.

James Peach and I had a discussion about this and thought that `off_t` is more 
correct for representing file sizes. Why would you prefer to stick with 
`size_t`?


> On Nov. 2, 2016, 10:52 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 3735
> > 
> >
> > While `vector::size_type` is the correct type here, we 
> > typically just use `size_t`.

Wouldn't `container::size_type` be more portable? `size_t` could vary on the 
platform where `size_type` is container dependent.


> On Nov. 2, 2016, 10:52 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/tests/io_tests.cpp, line 284
> > 
> >
> > `string::size_type` is the correct type, but we typically just use 
> > `size_t`.
> > 
> > Not directly an issue, but to me casting the signed LHS to an unsigned 
> > type feels more dangerous than casting the unsigned RHS to signed since I 
> > feel we seem much less likely deal with very large unsigned values (RHS) 
> > than with negative numbers close to zero like `-1` on the RHS. I would 
> > personally would cast the RHS instead. What do you think?

Same comment as above for your first comment.

I agree with what you're saying about the casting here. I'll swap it around.


- Aaron


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


On Oct. 27, 2016, 4:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 27, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-02 Thread Benjamin Bannier 

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


Fix it, then Ship it!





3rdparty/libprocess/src/decoder.hpp (line 243)


I wouldn't cast the signed RHS to unsigned here, but instead the unsigned 
LHS to signed, see more detailed note below.

Not originally yours, but one should really use 
`std::numeric_limits::max()` from `limits` here; alternatively this needs 
an `#include `



3rdparty/libprocess/src/decoder.hpp (line 438)


Let's not cast the signed RHS to unsigned here, but instead the unsigned 
LHS to signed.

Not originally yours, but one should really use 
`std::numeric_limits::max()` from `limits` here; alternatively this needs 
an `#include `



3rdparty/libprocess/src/encoder.hpp (line 290)


I think using an `off_t` for a size is semantically incorrect; I'd stay 
with `size_t`. This requires adjusting the usage above.



3rdparty/libprocess/src/process.cpp (line 3735)


While `vector::size_type` is the correct type here, we typically 
just use `size_t`.



3rdparty/libprocess/src/tests/io_tests.cpp (line 284)


`string::size_type` is the correct type, but we typically just use `size_t`.

Not directly an issue, but to me casting the signed LHS to an unsigned type 
feels more dangerous than casting the unsigned RHS to signed since I feel we 
seem much less likely deal with very large unsigned values (RHS) than with 
negative numbers close to zero like `-1` on the RHS. I would personally would 
cast the RHS instead. What do you think?


- Benjamin Bannier


On Oct. 27, 2016, 6:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 27, 2016, 6:51 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-11-01 Thread James Peach 

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


Ship it!




Ship It!

- James Peach


On Oct. 27, 2016, 4:51 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 27, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp ab2b5a9 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-27 Thread Aaron Wood 

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

(Updated Oct. 27, 2016, 4:51 p.m.)


Review request for mesos, James Peach, Michael Park, and Neil Conway.


Changes
---

Addressed comments about the encoder changes.


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


Repository: mesos


Description
---

The hardening flags produced many new sign comparison errors in libprocess that 
need to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp c79296b 
  3rdparty/libprocess/src/encoder.hpp 005d1cc 
  3rdparty/libprocess/src/process.cpp ab2b5a9 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
  3rdparty/libprocess/src/tests/http_tests.cpp 533104c 
  3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran `make && make check && make bench`.


Thanks,

Aaron Wood

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-27 Thread James Peach 


> On Oct. 18, 2016, 3:14 a.m., James Peach wrote:
> > 3rdparty/libprocess/src/tests/io_tests.cpp, line 284
> > 
> >
> > Can you just make ``length`` type ``ssize_t``?
> 
> Aaron Wood wrote:
> `length` is `ssize_t` (set on line 235)
> 
> James Peach wrote:
> Ok I see. It is bogus that ``length`` is ``ssize_t`` since ``io::read`` 
> is returning ``Future``. Just make ``length`` a ``size_t``.

Aaron and I discussed this. `length` needs to be `ssize_t` because it is the 
return from global `::write`. This means that casting `length` to an unsigned 
type is the most expedient solution. Alternatively, you could reuse `size` 
instead of `length`.


- James


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


On Oct. 21, 2016, 6:29 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 21, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp 18a8e20 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-24 Thread James Peach 


> On Oct. 18, 2016, 3:14 a.m., James Peach wrote:
> > 3rdparty/libprocess/src/tests/io_tests.cpp, line 284
> > 
> >
> > Can you just make ``length`` type ``ssize_t``?
> 
> Aaron Wood wrote:
> `length` is `ssize_t` (set on line 235)

Ok I see. It is bogus that ``length`` is ``ssize_t`` since ``io::read`` is 
returning ``Future``. Just make ``length`` a ``size_t``.


> On Oct. 18, 2016, 3:14 a.m., James Peach wrote:
> > 3rdparty/libprocess/src/encoder.hpp, line 291
> > 
> >
> > It's not obvious to me that this is the right change since the 
> > surrounding code tries to deal with ``off_t``. Can you explain this some 
> > more?
> 
> Aaron Wood wrote:
> In the `backup` method on line 276 it's being compared with a `size_t` as 
> well as in the `remaining` method on line 283. But in `next` on line 267 
> `size` which is a `size_t` is being casted to an `off_t` and then assigned to 
> `index`. 
> 
> The other encoders above in that file seem to be working with their own 
> private `index` as a `size_t` so even though it looks like it's being used in 
> different ways in `FileEncoder` I thought it should be a `size_t`.
> 
> Since this specific class is for files maybe we should keep it as `off_t` 
> and just cast the things used for comparisons to `off_t`.

``off_t`` is the "more correct" type for representing file sizes, but 
``FileEncoder`` seems a bit schizophrenic. I'd be inclined to keep both 
``size`` and ``index`` as ``off_t``, then cast ``length`` as appropriate.


- James


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


On Oct. 21, 2016, 6:29 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 21, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp 18a8e20 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-18 Thread Aaron Wood 


> On Oct. 18, 2016, 3:14 a.m., James Peach wrote:
> > 3rdparty/libprocess/src/tests/io_tests.cpp, line 284
> > 
> >
> > Can you just make ``length`` type ``ssize_t``?

`length` is `ssize_t` (set on line 235)


- Aaron


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


On Oct. 14, 2016, 3:14 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 14, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp 18a8e20 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-18 Thread Aaron Wood 


> On Oct. 18, 2016, 3:14 a.m., James Peach wrote:
> > 3rdparty/libprocess/src/encoder.hpp, line 291
> > 
> >
> > It's not obvious to me that this is the right change since the 
> > surrounding code tries to deal with ``off_t``. Can you explain this some 
> > more?

In the `backup` method on line 276 it's being compared with a `size_t` as well 
as in the `remaining` method on line 283. But in `next` on line 267 `size` 
which is a `size_t` is being casted to an `off_t` and then assigned to `index`. 

The other encoders above in that file seem to be working with their own private 
`index` as a `size_t` so even though it looks like it's being used in different 
ways in `FileEncoder` I thought it should be a `size_t`.

Since this specific class is for files maybe we should keep it as `off_t` and 
just cast the things used for comparisons to `off_t`.


- Aaron


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


On Oct. 14, 2016, 3:14 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 14, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp 18a8e20 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-17 Thread James Peach 

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




3rdparty/libprocess/src/encoder.hpp (line 291)


It's not obvious to me that this is the right change since the surrounding 
code tries to deal with ``off_t``. Can you explain this some more?



3rdparty/libprocess/src/tests/io_tests.cpp (line 284)


Can you just make ``length`` type ``ssize_t``?


- James Peach


On Oct. 14, 2016, 3:14 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 14, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp 18a8e20 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

2016-10-14 Thread Aaron Wood 

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

(Updated Oct. 14, 2016, 3:14 p.m.)


Review request for mesos, Michael Park and Neil Conway.


Changes
---

Targeted this RR at libprocess. There will be a separate one opened for stout.


Summary (updated)
-

Fix new sign comparison errors in libprocess produced by hardened flags


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


Repository: mesos


Description (updated)
---

The hardening flags produced many new sign comparison errors in libprocess that 
need to be fixed for Mesos to compile/run.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp c79296b 
  3rdparty/libprocess/src/encoder.hpp 005d1cc 
  3rdparty/libprocess/src/process.cpp 18a8e20 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
  3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
  3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 

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


Testing
---

Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
Ran `make && make check && make bench`.


Thanks,

Aaron Wood