Re: Review Request 49559: Added ELFIO as bundled dependency in Mesos.

[Kevin Klues]

> On July 2, 2016, 9:51 p.m., Benjamin Mahler wrote:
> > 3rdparty/Makefile.am, line 175
> > 
> >
> > Looks like we need the following:
> > 
> > ```
> > $(ELFIO)/elfio/elf_types.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_dump.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_dynamic.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_header.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_note.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_relocation.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_section.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_segment.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_strings.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_symbols.hpp: $(ELFIO)-stamp
> > $(ELFIO)/elfio/elfio_utils.hpp: $(ELFIO)-stamp
> > ```

Strange. It built fine for me. Then again, I run with `make -j`, so maybe the 
stamp got created from another target for me concurrently, so I didn't notice. 
At one point I was using the target:

`$(ELFIO)/elfio/%.hpp`

But it didn't like that for some reason.


- Kevin


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


On July 2, 2016, 8:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49559/
> ---
> 
> (Updated July 2, 2016, 8:08 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5767
> https://issues.apache.org/jira/browse/MESOS-5767
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes a patch file to ELFIO to fix 2 off-by-one errors in the
> parsing of NOTE sections. Patches for these bugs have been submitted
> upstream. Details of the patches are below:
> 
> 1) Fixed off-by-one error in 'name' of add_note() function.
> 
> Previously, when assigning 'name' as a string, it's length was
> specified using the full length of 'namesz'. However, this length
> includes the trailing '\0' of the underlying char[]. This ultimately
> causes the C++ string that is created to (incorrectly) contain the
> '\0' character as well. This leads to problems where e.g. the
> following will return false, even when 'name' itself contains the
> string "GNU\0":
> 
>   if (name == "GNU") {
> return true;
>   }
>   return false;
> 
> To fix this, we should only include the length of the string minus the
> trailing '\0'.
> 
> 2) Fixed alignment of 'desc' in add_note() function.
> 
> The ELF spec specifically lists the alignment of the namez char[] to
> be 4 bytes. To quote it:
> 
> "Padding is present, if necessary, to ensure 4-byte alignment for the
> descriptor. Such padding is not included in namesz."
> 
> However, the current implementation sets the alignment to either 4 or
> 8 bytes depending on the class of the ELF file (CLASS32 or CLASS64).
> This commit fixes the alignment to only 4 bytes in all cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 15f3171 
>   3rdparty/Makefile.am bd990cc 
>   3rdparty/cmake/Versions.cmake 7b73f8f 
>   3rdparty/elfio-3.1.patch PRE-CREATION 
>   3rdparty/elfio-3.1.tar.gz PRE-CREATION 
>   3rdparty/versions.am 203656c 
>   LICENSE eb39f6d 
>   configure.ac 321436b 
>   src/Makefile.am 52d63f2 
>   support/coverage.sh ab9564b 
> 
> Diff: https://reviews.apache.org/r/49559/diff/
> 
> 
> Testing
> ---
> 
> Made sure `elfio-3.1` appears in `/build` after running `../configure`, 
> `make`.
> Made sure `elfio` appears in `/include` of installation folder after `make 
> install`.
> 
> This second one is necessary because `stout` relies on elfio in order to 
> function (similar to how we've done with `picojson.h` in the past).
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49559: Added ELFIO as bundled dependency in Mesos.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/Makefile.am (line 175)


Looks like we need the following:

```
$(ELFIO)/elfio/elf_types.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_dump.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_dynamic.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_header.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_note.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_relocation.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_section.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_segment.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_strings.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_symbols.hpp: $(ELFIO)-stamp
$(ELFIO)/elfio/elfio_utils.hpp: $(ELFIO)-stamp
```


- Benjamin Mahler


On July 2, 2016, 8:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49559/
> ---
> 
> (Updated July 2, 2016, 8:08 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5767
> https://issues.apache.org/jira/browse/MESOS-5767
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes a patch file to ELFIO to fix 2 off-by-one errors in the
> parsing of NOTE sections. Patches for these bugs have been submitted
> upstream. Details of the patches are below:
> 
> 1) Fixed off-by-one error in 'name' of add_note() function.
> 
> Previously, when assigning 'name' as a string, it's length was
> specified using the full length of 'namesz'. However, this length
> includes the trailing '\0' of the underlying char[]. This ultimately
> causes the C++ string that is created to (incorrectly) contain the
> '\0' character as well. This leads to problems where e.g. the
> following will return false, even when 'name' itself contains the
> string "GNU\0":
> 
>   if (name == "GNU") {
> return true;
>   }
>   return false;
> 
> To fix this, we should only include the length of the string minus the
> trailing '\0'.
> 
> 2) Fixed alignment of 'desc' in add_note() function.
> 
> The ELF spec specifically lists the alignment of the namez char[] to
> be 4 bytes. To quote it:
> 
> "Padding is present, if necessary, to ensure 4-byte alignment for the
> descriptor. Such padding is not included in namesz."
> 
> However, the current implementation sets the alignment to either 4 or
> 8 bytes depending on the class of the ELF file (CLASS32 or CLASS64).
> This commit fixes the alignment to only 4 bytes in all cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 15f3171 
>   3rdparty/Makefile.am bd990cc 
>   3rdparty/cmake/Versions.cmake 7b73f8f 
>   3rdparty/elfio-3.1.patch PRE-CREATION 
>   3rdparty/elfio-3.1.tar.gz PRE-CREATION 
>   3rdparty/versions.am 203656c 
>   LICENSE eb39f6d 
>   configure.ac 321436b 
>   src/Makefile.am 52d63f2 
>   support/coverage.sh ab9564b 
> 
> Diff: https://reviews.apache.org/r/49559/diff/
> 
> 
> Testing
> ---
> 
> Made sure `elfio-3.1` appears in `/build` after running `../configure`, 
> `make`.
> Made sure `elfio` appears in `/include` of installation folder after `make 
> install`.
> 
> This second one is necessary because `stout` relies on elfio in order to 
> function (similar to how we've done with `picojson.h` in the past).
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49559: Added ELFIO as bundled dependency in Mesos.

[Kevin Klues]

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

(Updated July 2, 2016, 8:08 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Uploaded binary diff for `elfio-3.1.tar.gz`


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


Repository: mesos


Description
---

This includes a patch file to ELFIO to fix 2 off-by-one errors in the
parsing of NOTE sections. Patches for these bugs have been submitted
upstream. Details of the patches are below:

1) Fixed off-by-one error in 'name' of add_note() function.

Previously, when assigning 'name' as a string, it's length was
specified using the full length of 'namesz'. However, this length
includes the trailing '\0' of the underlying char[]. This ultimately
causes the C++ string that is created to (incorrectly) contain the
'\0' character as well. This leads to problems where e.g. the
following will return false, even when 'name' itself contains the
string "GNU\0":

  if (name == "GNU") {
return true;
  }
  return false;

To fix this, we should only include the length of the string minus the
trailing '\0'.

2) Fixed alignment of 'desc' in add_note() function.

The ELF spec specifically lists the alignment of the namez char[] to
be 4 bytes. To quote it:

"Padding is present, if necessary, to ensure 4-byte alignment for the
descriptor. Such padding is not included in namesz."

However, the current implementation sets the alignment to either 4 or
8 bytes depending on the class of the ELF file (CLASS32 or CLASS64).
This commit fixes the alignment to only 4 bytes in all cases.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 15f3171 
  3rdparty/Makefile.am bd990cc 
  3rdparty/cmake/Versions.cmake 7b73f8f 
  3rdparty/elfio-3.1.patch PRE-CREATION 
  3rdparty/elfio-3.1.tar.gz PRE-CREATION 
  3rdparty/versions.am 203656c 
  LICENSE eb39f6d 
  configure.ac 321436b 
  src/Makefile.am 52d63f2 
  support/coverage.sh ab9564b 

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


Testing
---

Made sure `elfio-3.1` appears in `/build` after running `../configure`, `make`.
Made sure `elfio` appears in `/include` of installation folder after `make 
install`.

This second one is necessary because `stout` relies on elfio in order to 
function (similar to how we've done with `picojson.h` in the past).


Thanks,

Kevin Klues

Review Request 49559: Added ELFIO as bundled dependency in Mesos.

[Kevin Klues]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This includes a patch file to ELFIO to fix 2 off-by-one errors in the
parsing of NOTE sections. Patches for these bugs have been submitted
upstream. Details of the patches are below:

1) Fixed off-by-one error in 'name' of add_note() function.

Previously, when assigning 'name' as a string, it's length was
specified using the full length of 'namesz'. However, this length
includes the trailing '\0' of the underlying char[]. This ultimately
causes the C++ string that is created to (incorrectly) contain the
'\0' character as well. This leads to problems where e.g. the
following will return false, even when 'name' itself contains the
string "GNU\0":

  if (name == "GNU") {
return true;
  }
  return false;

To fix this, we should only include the length of the string minus the
trailing '\0'.

2) Fixed alignment of 'desc' in add_note() function.

The ELF spec specifically lists the alignment of the namez char[] to
be 4 bytes. To quote it:

"Padding is present, if necessary, to ensure 4-byte alignment for the
descriptor. Such padding is not included in namesz."

However, the current implementation sets the alignment to either 4 or
8 bytes depending on the class of the ELF file (CLASS32 or CLASS64).
This commit fixes the alignment to only 4 bytes in all cases.


Diffs
-

  3rdparty/CMakeLists.txt 15f3171bb8c7ab1ca28b047a552e80492dd1b157 
  3rdparty/Makefile.am bd990cc6298c6b564df8f152673d42a2a979fb55 
  3rdparty/cmake/Versions.cmake 7b73f8f4a0c4180f7ed037efb1076f967981f2f0 
  3rdparty/elfio-3.1.patch PRE-CREATION 
  3rdparty/elfio-3.1.tar.gz PRE-CREATION 
  3rdparty/versions.am 203656cd1c55576dd9d6302dfa7e076434ef089b 
  LICENSE eb39f6d69a165f59c00e8bb0ba9e15be8c958a5b 
  configure.ac 321436beb8ad87bb5727932eb2943986fe558237 
  src/Makefile.am 52d63f26e0455ef31411e964e497a509d96aaf4a 
  support/coverage.sh ab9564bc1b921da64c70c14915067c5d3e683808 

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


Testing
---

Made sure `elfio-3.1` appears in `/build` after running `../configure`, `make`.
Made sure `elfio` appears in `/include` of installation folder after `make 
install`.

This second one is necessary because `stout` relies on elfio in order to 
function (similar to how we've done with `picojson.h` in the past).


Thanks,

Kevin Klues