You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2021/05/13 19:48:03 UTC
[GitHub] [mesos] cf-natali opened a new pull request #384: Fixed parsing of ld.so.cache on new glibc.
cf-natali opened a new pull request #384:
URL: https://github.com/apache/mesos/pull/384
Since glibc 2.32, `ld.so.cache` now defaults to the "new" format, instead
of the "compat" format which was in use since glibc 2.2 (around 20 years
ago).
The code now supports both the "compat" and "new" formats:
Before:
```
root@thinkpad:/home/cf/src/mesos/build# ldconfig -c new
root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld*
[...]
[==========] Running 4 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from LdcacheTest
[ RUN ] LdcacheTest.Parse
../../src/tests/ldcache_tests.cpp:43: Failure
cache: Invalid format
[ FAILED ] LdcacheTest.Parse (0 ms)
[----------] 1 test from LdcacheTest (0 ms total)
[----------] 3 tests from Ldd
[ RUN ] Ldd.BinSh
../../src/tests/ldd_tests.cpp:43: Failure
cache: Invalid format
[ FAILED ] Ldd.BinSh (0 ms)
[ RUN ] Ldd.EmptyCache
[ OK ] Ldd.EmptyCache (1 ms)
[ RUN ] Ldd.MissingFile
../../src/tests/ldd_tests.cpp:77: Failure
cache: Invalid format
[ FAILED ] Ldd.MissingFile (0 ms)
[----------] 3 tests from Ldd (1 ms total)
[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (8 ms total)
[ PASSED ] 1 test.
[ FAILED ] 3 tests, listed below:
[ FAILED ] LdcacheTest.Parse
[ FAILED ] Ldd.BinSh
[ FAILED ] Ldd.MissingFile
3 FAILED TESTS
```
After:
```
root@thinkpad:/home/cf/src/mesos/build# ldconfig -c new
root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld*
[...]
[==========] Running 4 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from LdcacheTest
[ RUN ] LdcacheTest.Parse
[ OK ] LdcacheTest.Parse (529 ms)
[----------] 1 test from LdcacheTest (529 ms total)
[----------] 3 tests from Ldd
[ RUN ] Ldd.BinSh
[ OK ] Ldd.BinSh (3 ms)
[ RUN ] Ldd.EmptyCache
[ OK ] Ldd.EmptyCache (0 ms)
[ RUN ] Ldd.MissingFile
[ OK ] Ldd.MissingFile (0 ms)
[----------] 3 tests from Ldd (3 ms total)
[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (541 ms total)
[ PASSED ] 4 tests.
```
Closes #10220.
@asekretenko @andreaspeters
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850890927
Ah and by the way, here's how I tested it:
`compat` format - until 2.32/2.31:
```
root@thinkpad:/home/cf/src/mesos/build# ldconfig -c compat
root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld* 2>&1 | tail -n13
[----------] 3 tests from Ldd
[ RUN ] Ldd.BinSh
[ OK ] Ldd.BinSh (2 ms)
[ RUN ] Ldd.EmptyCache
[ OK ] Ldd.EmptyCache (0 ms)
[ RUN ] Ldd.MissingFile
[ OK ] Ldd.MissingFile (1 ms)
[----------] 3 tests from Ldd (3 ms total)
[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (548 ms total)
[ PASSED ] 4 tests.
```
`new` format - supported by this change:
```
root@thinkpad:/home/cf/src/mesos/build# ldconfig -c new
root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld* 2>&1 | tail -n13
[----------] 3 tests from Ldd
[ RUN ] Ldd.BinSh
[ OK ] Ldd.BinSh (3 ms)
[ RUN ] Ldd.EmptyCache
[ OK ] Ldd.EmptyCache (0 ms)
[ RUN ] Ldd.MissingFile
[ OK ] Ldd.MissingFile (1 ms)
[----------] 3 tests from Ldd (4 ms total)
[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (550 ms total)
[ PASSED ] 4 tests.
```
`old` format - never supported - as a sanity check:
```
root@thinkpad:/home/cf/src/mesos/build# ldconfig -c old
root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld* 2>&1 | tail -n13
cache: Invalid format
[ FAILED ] Ldd.MissingFile (0 ms)
[----------] 3 tests from Ldd (1 ms total)
[----------] Global test environment tear-down
[==========] 4 tests from 2 test cases ran. (7 ms total)
[ PASSED ] 1 test.
[ FAILED ] 3 tests, listed below:
[ FAILED ] LdcacheTest.Parse
[ FAILED ] Ldd.BinSh
[ FAILED ] Ldd.MissingFile
3 FAILED TESTS
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-842561741
FWIW it's breaking other things:
```
[ RUN ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage
../../src/tests/health_check_tests.cpp:505: Failure
(testImage).failure(): Failed to create docker test image rootfs: Failed to parse ld.so cache: Invalid format
[ FAILED ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage (26 ms)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850833193
> FWIW it's breaking other things:
>
> ```
> [ RUN ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage
> ../../src/tests/health_check_tests.cpp:505: Failure
> (testImage).failure(): Failed to create docker test image rootfs: Failed to parse ld.so cache: Invalid format
> [ FAILED ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage (26 ms)
> ```
@cf-natali Did you run this test in the same machine with the other tests (like `LdcacheTest.Parse`), all of these tests will call `ldcache::parse()` internally, so it does not make sense that one succeeds but the other fails.
In another hand, there are a couple of places in `ldcache::parse()` which will error out with `Invalid format`, maybe we should use distinct error message in each places so that we can better troubleshooting this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali edited a comment on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali edited a comment on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850890266
> > Dunno, the issue seems pretty clear to me, it affects anything parsing ldcache due to the format change. And the attached change fixes it.
>
> NVM, I thought `HealthCheckTest.ROOT_HealthyTaskWithContainerImage` failed after your code changes in this PR was applied, sorry for the confusion.
Aha no worries it happens!
I think all comments are addressed now, let me know if you need anything else :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] asfgit closed pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #384:
URL: https://github.com/apache/mesos/pull/384
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-846120117
@bmahler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa edited a comment on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa edited a comment on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850833193
> FWIW it's breaking other things:
>
> ```
> [ RUN ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage
> ../../src/tests/health_check_tests.cpp:505: Failure
> (testImage).failure(): Failed to create docker test image rootfs: Failed to parse ld.so cache: Invalid format
> [ FAILED ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage (26 ms)
> ```
@cf-natali Did you run this test in the same machine with the other tests (like `LdcacheTest.Parse`), all of these tests will call `ldcache::parse()` internally, so it does not make sense that one succeeds but the other fails.
In another hand, there are a couple of places in `ldcache::parse()` which will error out with `Invalid format`, maybe we should use distinct error message in each place so that we can better troubleshooting this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #384:
URL: https://github.com/apache/mesos/pull/384#discussion_r641978207
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,49 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
+ // If the data doesn't start with the new header, it must be a
+ // compat format, therefore we expect an old header.
+ HeaderOld* headerOld = (HeaderOld*)data;
+ data += sizeof(HeaderOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
- // The new format header and all of its library entries are embedded
- // in the old format's string table (the current location of data).
- // However, the header is aligned on an 8 byte boundary, so we
- // need to align 'data' to get it to point to the new header.
- data = align(data, alignof(HeaderNew));
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
+ // Validate our header magic.
+ if (strncmp(headerOld->magic,
+ HEADER_MAGIC_OLD,
+ sizeof(HEADER_MAGIC_OLD) - 1) != 0) {
+ return Error("Invalid format");
+ }
+
+ data += headerOld->libraryCount * sizeof(EntryOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+
+ // The new format header and all of its library entries are
+ // embedded in the old format's string table (the current
+ // location of data).
+ // However, the header is aligned on an 8 byte boundary, so we
+ // need to align 'data' to get it to point to the new header.
+ data = align(data, alignof(HeaderNew));
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
}
// Construct pointers to all of the important regions in the new
// format: the header, the libentry array, and the new string table
// (which starts at the same address as the aligned headerNew pointer).
- HeaderNew* headerNew = (HeaderNew*)data;
+ headerNew = (HeaderNew*)data;
Review comment:
Yep I hesitated to do this when I initially wrote it but kept it since I thought it was clearer to see where the `headerNew` is set, but I'm fine either way, so did the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-842074878
For me it looks fine. How about @asekretenko ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-842074878
For me it looks fine. How about @asekretenko ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850840426
On Sat, 29 May 2021, 14:24 Qian Zhang, ***@***.***> wrote:
> FWIW it's breaking other things:
>
> [ RUN ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage
> ../../src/tests/health_check_tests.cpp:505: Failure
> (testImage).failure(): Failed to create docker test image rootfs: Failed to parse ld.so cache: Invalid format
> [ FAILED ] HealthCheckTest.ROOT_HealthyTaskWithContainerImage (26 ms)
>
> @cf-natali <https://github.com/cf-natali> Did you run this test in the
> same machine with the other tests (like LdcacheTest.Parse), all of these
> tests will call ldcache::parse() internally, so it does not make sense
> that one succeeds but the other fails.
>
I'm not sure what you mean - this test is an example of failing tests,
among many others.
In another hand, there are a couple of places in ldcache::parse() which
> will error out with Invalid format, maybe we should use distinct error
> message in each places so that we can better troubleshooting this issue.
>
Dunno, the issue seems pretty clear to me, it affects anything parsing
ldcache due to the format change. And the attached change fixes it.
—
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/mesos/pull/384#issuecomment-850833193>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB75O43IY5HF3SVBGXFFSS3TQDTIZANCNFSM443EOPDA>
> .
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-847206811
@qianzhangxa
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #384:
URL: https://github.com/apache/mesos/pull/384#discussion_r641965468
##########
File path: src/linux/ldcache.cpp
##########
@@ -79,8 +81,22 @@ using std::vector;
// ^
// end of string table
//
-// We currently only support the new format, since glibc 2.2
-// was released in late 2000.
+// Then from glibc 2.32 the default changed to the new format, without
Review comment:
Interesting!
Here's the reference I can find: https://sourceware.org/bugzilla/show_bug.cgi?id=23668
Which does mention 2.32.
However my Debian has version 2.31, and also defaults to the new format:
```
cf@thinkpad:~$ /sbin/ldconfig --help | grep format
-c, --format=FORMAT Format to use: new (default), old, or compat
cf@thinkpad:~$ ldd --version | head -n1
ldd (Debian GLIBC 2.31-12) 2.31
```
I'm not sure why - it's possible that Debian and Ubuntu backported the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-851005111
> I think all comments are addressed now, let me know if you need anything else :).
Just posted a minor comment, otherwise I think we are good to merge :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on a change in pull request #384:
URL: https://github.com/apache/mesos/pull/384#discussion_r641943724
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,49 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
Review comment:
Not yours, the indent here seems not correct, it should be:
```
if (strncmp(headerNew->magic,
HEADER_MAGIC_NEW,
sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
```
See [this code](https://github.com/apache/mesos/blob/1.11.0/src/slave/containerizer/mesos/containerizer.cpp#L599:L601) as an example. Ditto for the other `strncmp` below.
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,49 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
+ // If the data doesn't start with the new header, it must be a
+ // compat format, therefore we expect an old header.
+ HeaderOld* headerOld = (HeaderOld*)data;
+ data += sizeof(HeaderOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
- // The new format header and all of its library entries are embedded
- // in the old format's string table (the current location of data).
- // However, the header is aligned on an 8 byte boundary, so we
- // need to align 'data' to get it to point to the new header.
- data = align(data, alignof(HeaderNew));
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
+ // Validate our header magic.
+ if (strncmp(headerOld->magic,
+ HEADER_MAGIC_OLD,
+ sizeof(HEADER_MAGIC_OLD) - 1) != 0) {
+ return Error("Invalid format");
+ }
+
+ data += headerOld->libraryCount * sizeof(EntryOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+
+ // The new format header and all of its library entries are
+ // embedded in the old format's string table (the current
+ // location of data).
Review comment:
I think these 3 lines should be merged into 2 lines.
##########
File path: src/linux/ldcache.cpp
##########
@@ -79,8 +81,22 @@ using std::vector;
// ^
// end of string table
//
-// We currently only support the new format, since glibc 2.2
-// was released in late 2000.
+// Then from glibc 2.32 the default changed to the new format, without
Review comment:
Can you please let me know the reference of the default changed to the new format starting from glibc 2.32? It seems the default has already been changed to new for glibc 2.31 (Ubuntu 20.04.2):
```
$ ldconfig --help | grep format
-c, --format=FORMAT Format to use: new (default), old, or compat
$ ldd --version
ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31
```
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,49 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
+ // If the data doesn't start with the new header, it must be a
+ // compat format, therefore we expect an old header.
+ HeaderOld* headerOld = (HeaderOld*)data;
+ data += sizeof(HeaderOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
- // The new format header and all of its library entries are embedded
- // in the old format's string table (the current location of data).
- // However, the header is aligned on an 8 byte boundary, so we
- // need to align 'data' to get it to point to the new header.
- data = align(data, alignof(HeaderNew));
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
+ // Validate our header magic.
+ if (strncmp(headerOld->magic,
+ HEADER_MAGIC_OLD,
+ sizeof(HEADER_MAGIC_OLD) - 1) != 0) {
+ return Error("Invalid format");
+ }
+
+ data += headerOld->libraryCount * sizeof(EntryOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+
+ // The new format header and all of its library entries are
+ // embedded in the old format's string table (the current
+ // location of data).
+ // However, the header is aligned on an 8 byte boundary, so we
+ // need to align 'data' to get it to point to the new header.
+ data = align(data, alignof(HeaderNew));
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
}
// Construct pointers to all of the important regions in the new
// format: the header, the libentry array, and the new string table
// (which starts at the same address as the aligned headerNew pointer).
- HeaderNew* headerNew = (HeaderNew*)data;
+ headerNew = (HeaderNew*)data;
Review comment:
Better to move this line into the above `if` block (i.e. between line 201 and 202)? Otherwise for the case of new format we will set `headerNew` twice (the other one is line 165).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #384:
URL: https://github.com/apache/mesos/pull/384#discussion_r641978062
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,49 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
+ // If the data doesn't start with the new header, it must be a
+ // compat format, therefore we expect an old header.
+ HeaderOld* headerOld = (HeaderOld*)data;
+ data += sizeof(HeaderOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
- // The new format header and all of its library entries are embedded
- // in the old format's string table (the current location of data).
- // However, the header is aligned on an 8 byte boundary, so we
- // need to align 'data' to get it to point to the new header.
- data = align(data, alignof(HeaderNew));
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
+ // Validate our header magic.
+ if (strncmp(headerOld->magic,
+ HEADER_MAGIC_OLD,
+ sizeof(HEADER_MAGIC_OLD) - 1) != 0) {
+ return Error("Invalid format");
+ }
+
+ data += headerOld->libraryCount * sizeof(EntryOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+
+ // The new format header and all of its library entries are
+ // embedded in the old format's string table (the current
+ // location of data).
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on a change in pull request #384:
URL: https://github.com/apache/mesos/pull/384#discussion_r642080925
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,48 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
+ // If the data doesn't start with the new header, it must be a
+ // compat format, therefore we expect an old header.
+ HeaderOld* headerOld = (HeaderOld*)data;
+ data += sizeof(HeaderOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
- // The new format header and all of its library entries are embedded
- // in the old format's string table (the current location of data).
- // However, the header is aligned on an 8 byte boundary, so we
- // need to align 'data' to get it to point to the new header.
- data = align(data, alignof(HeaderNew));
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
+ // Validate our header magic.
+ if (strncmp(headerOld->magic,
+ HEADER_MAGIC_OLD,
+ sizeof(HEADER_MAGIC_OLD) - 1) != 0) {
+ return Error("Invalid format");
+ }
+
+ data += headerOld->libraryCount * sizeof(EntryOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+
+ // The new format header and all of its library entries are embedded
+ // in the old format's string table (the current location of data).
+ // However, the header is aligned on an 8 byte boundary, so we
+ // need to align 'data' to get it to point to the new header.
+ data = align(data, alignof(HeaderNew));
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+ headerNew = (HeaderNew*)data;
Review comment:
A newline between.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850890266
> > Dunno, the issue seems pretty clear to me, it affects anything parsing ldcache due to the format change. And the attached change fixes it.
>
> NVM, I thought `HealthCheckTest.ROOT_HealthyTaskWithContainerImage` failed after your code changes in this PR was applied, sorry for the confusion.
Aha no worries it happens!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-850844432
> Dunno, the issue seems pretty clear to me, it affects anything parsing ldcache due to the format change. And the attached change fixes it.
NVM, I thought `HealthCheckTest.ROOT_HealthyTaskWithContainerImage` failed after your code changes in this PR was applied, sorry for the confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #384:
URL: https://github.com/apache/mesos/pull/384#discussion_r642112754
##########
File path: src/linux/ldcache.cpp
##########
@@ -146,33 +162,48 @@ Try<vector<Entry>> parse(const string& path)
const char* data = buffer->data();
- // Grab a pointer to the old format header (for verification of
- // HEADER_MAGIC_OLD later on). Then jump forward to the location of
- // the new format header (it is the only format we support).
- HeaderOld* headerOld = (HeaderOld*)data;
- data += sizeof(HeaderOld);
- if (data >= buffer->data() + buffer->size()) {
+ HeaderNew* headerNew = (HeaderNew*)data;
+ if (data + sizeof(HeaderNew) >= buffer->data() + buffer->size()) {
return Error("Invalid format");
}
- data += headerOld->libraryCount * sizeof(EntryOld);
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
- }
+ if (strncmp(headerNew->magic,
+ HEADER_MAGIC_NEW,
+ sizeof(HEADER_MAGIC_NEW) - 1) != 0) {
+ // If the data doesn't start with the new header, it must be a
+ // compat format, therefore we expect an old header.
+ HeaderOld* headerOld = (HeaderOld*)data;
+ data += sizeof(HeaderOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
- // The new format header and all of its library entries are embedded
- // in the old format's string table (the current location of data).
- // However, the header is aligned on an 8 byte boundary, so we
- // need to align 'data' to get it to point to the new header.
- data = align(data, alignof(HeaderNew));
- if (data >= buffer->data() + buffer->size()) {
- return Error("Invalid format");
+ // Validate our header magic.
+ if (strncmp(headerOld->magic,
+ HEADER_MAGIC_OLD,
+ sizeof(HEADER_MAGIC_OLD) - 1) != 0) {
+ return Error("Invalid format");
+ }
+
+ data += headerOld->libraryCount * sizeof(EntryOld);
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+
+ // The new format header and all of its library entries are embedded
+ // in the old format's string table (the current location of data).
+ // However, the header is aligned on an 8 byte boundary, so we
+ // need to align 'data' to get it to point to the new header.
+ data = align(data, alignof(HeaderNew));
+ if (data >= buffer->data() + buffer->size()) {
+ return Error("Invalid format");
+ }
+ headerNew = (HeaderNew*)data;
Review comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #384: Fixed parsing of ld.so.cache on new glibc.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #384:
URL: https://github.com/apache/mesos/pull/384#issuecomment-851495709
The commit in this PR has been merged, and I also marked https://issues.apache.org/jira/browse/MESOS-10220 as resolved, thanks @cf-natali for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org