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/29 14:30:49 UTC
[GitHub] [mesos] qianzhangxa commented on a change in pull request #384: Fixed parsing of ld.so.cache on new glibc.
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