Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7377008:
Implement mapping randomization for 64-bit Linux. Notes: (Closed)
|
Created:
9 years, 5 months ago by Chris Evans Modified:
9 years, 5 months ago Reviewers:
Mads Ager (google), Vyacheslav Egorov (Chromium), William Hesse, Mads Ager (chromium), Cris Neckar CC:
v8-dev Visibility:
Public. |
DescriptionImplement mapping randomization for 64-bit Linux. Notes:
- 32-bit Linux already seems to scatter the mmap() chunks around; 64-bit didn't.
- Seed the system random number generator a little better (we needlessly
trunctaed microsecond resolution to millisecond resolution).
- Will automatically take advantage of better entropy when V8::RandomPrivate
uses it.
BUG=v8:805
Committed: http://code.google.com/p/v8/source/detail?r=8702
Patch Set 1 #
Total comments: 8
Patch Set 2 : '' #Patch Set 3 : '' #Messages
Total messages: 9 (0 generated)
Supercedes http://codereview.chromium.org/7374005/ Passes tests.
LGTM with comments addressed. http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc File src/platform-linux.cc (right): http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode86 src/platform-linux.cc:86: if (sizeof(void*) == 8 && (isolate = Isolate::UncheckedCurrent())) { I would prefer conditional compilation based on V8_TARGET_ARCH_X64 here instead of sizeof(void*) comparison. Also we prefer explicit comparisons with NULL to implicit to boolean conversion: Isolate* isolate = Isolate::UncheckedCurrent(); if (isolate != NULL) ... http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode109 src/platform-linux.cc:109: uint64_t seed = Ticks() | (getpid() << 16); I think the comment above needs updating.
More comments. http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc File src/platform-linux.cc (right): http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode89 src/platform-linux.cc:89: uint64_t raw_addr = (static_cast<uint64_t>(rnd1) << 33) | I think it should be 34 not 33. http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode93 src/platform-linux.cc:93: raw_addr <<= 18; Look into globals.h. There are macroses that allow to define 64bit constants.
http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc File src/platform-linux.cc (left): http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#oldcode374 src/platform-linux.cc:374: if (mbase == MAP_FAILED) { I think you need to add the MAP_VARIABLE flag to the flags, once you start supplying a non-null address to mmap. Otherwise, the function can fail due to conflict and return MAP_FAILED, even when memory is available. It may be that MAP_VARIABLE is actually a 0 flag, which is why it is working currently. http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc File src/platform-linux.cc (right): http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode89 src/platform-linux.cc:89: uint64_t raw_addr = (static_cast<uint64_t>(rnd1) << 33) | All of the masking can be replaced by raw_addr = (static_cast<uint64_t>(rnd1) << 14) ^ (static_cast<uint64_t>(rnd2) << 12) This has 0s in the upper 18 and lower 12 bits. You should not combine random numbers with OR unless they are non-overlapping bits. A random bit OR another random bit is 1 with probability 3/4. Use XOR instead. http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode92 src/platform-linux.cc:92: // permit the use of a 64-bit constant). There are macros in globals.h to enter 64-bit constants into code (on both 32-bit and 64-bit platforms). http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode108 src/platform-linux.cc:108: // call this setup code within the same millisecond. Use XOR, not OR, to combine the overlapping seed sources.
On 2011/07/15 12:43:22, William Hesse wrote: > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc > File src/platform-linux.cc (left): > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#oldcode374 > src/platform-linux.cc:374: if (mbase == MAP_FAILED) { > I think you need to add the MAP_VARIABLE flag to the flags, once you start > supplying a non-null address to mmap. Otherwise, the function can fail due to > conflict and return MAP_FAILED, even when memory is available. It may be that > MAP_VARIABLE is actually a 0 flag, which is why it is working currently. Linux doesn't have MAP_VARIABLE; the contract is the presence vs. absence of MAP_FIXED. (Quick test to demo safety): mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f16b46be000 mmap(0x7f16b46be000, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f16b46bd000 > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc > File src/platform-linux.cc (right): > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode89 > src/platform-linux.cc:89: uint64_t raw_addr = (static_cast<uint64_t>(rnd1) << > 33) | > All of the masking can be replaced by > raw_addr = (static_cast<uint64_t>(rnd1) << 14) ^ > (static_cast<uint64_t>(rnd2) << 12) Thanks all for the comments on this part. I've replaced it with something much simpler. > > This has 0s in the upper 18 and lower 12 bits. > You should not combine random numbers with OR unless they > are non-overlapping bits. A random bit OR another random bit is 1 with > probability 3/4. Use XOR instead. > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode92 > src/platform-linux.cc:92: // permit the use of a 64-bit constant). > There are macros in globals.h to enter 64-bit constants into code (on both > 32-bit and 64-bit platforms). > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode108 > src/platform-linux.cc:108: // call this setup code within the same millisecond. > Use XOR, not OR, to combine the overlapping seed sources. Oh, I'm embarrassed about that one. Thanks. I'll upload a patch will all review comments from everyone actioned, shortly.
Ok, comments addressed. Re-ran tests (ok). Checked compile of 32-bit Linux too. PTAL? If you like it, can you land it? On 2011/07/15 11:36:23, Vyacheslav Egorov wrote: > LGTM with comments addressed. > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc > File src/platform-linux.cc (right): > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode86 > src/platform-linux.cc:86: if (sizeof(void*) == 8 && (isolate = > Isolate::UncheckedCurrent())) { > I would prefer conditional compilation based on V8_TARGET_ARCH_X64 here instead > of sizeof(void*) comparison. > > Also we prefer explicit comparisons with NULL to implicit to boolean conversion: > > Isolate* isolate = Isolate::UncheckedCurrent(); > if (isolate != NULL) ... > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode109 > src/platform-linux.cc:109: uint64_t seed = Ticks() | (getpid() << 16); > I think the comment above needs updating.
On 2011/07/18 21:12:57, Chris Evans wrote: > Ok, comments addressed. > Re-ran tests (ok). Checked compile of 32-bit Linux too. > PTAL? > If you like it, can you land it? Actually, gimme 30 mins.... I'm going to do something for 32-bit too based on non-ideal behavior I just observed on a PAE kernel. > > On 2011/07/15 11:36:23, Vyacheslav Egorov wrote: > > LGTM with comments addressed. > > > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc > > File src/platform-linux.cc (right): > > > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode86 > > src/platform-linux.cc:86: if (sizeof(void*) == 8 && (isolate = > > Isolate::UncheckedCurrent())) { > > I would prefer conditional compilation based on V8_TARGET_ARCH_X64 here > instead > > of sizeof(void*) comparison. > > > > Also we prefer explicit comparisons with NULL to implicit to boolean > conversion: > > > > Isolate* isolate = Isolate::UncheckedCurrent(); > > if (isolate != NULL) ... > > > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode109 > > src/platform-linux.cc:109: uint64_t seed = Ticks() | (getpid() << 16); > > I think the comment above needs updating.
Ok, 32-bit support added. Really helps for ChromeOS as it happens. Definitely something we want. On 2011/07/20 19:54:12, Chris Evans wrote: > On 2011/07/18 21:12:57, Chris Evans wrote: > > Ok, comments addressed. > > Re-ran tests (ok). Checked compile of 32-bit Linux too. > > PTAL? > > If you like it, can you land it? > > Actually, gimme 30 mins.... I'm going to do something for 32-bit too based on > non-ideal behavior I just observed on a PAE kernel. > > > > > On 2011/07/15 11:36:23, Vyacheslav Egorov wrote: > > > LGTM with comments addressed. > > > > > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc > > > File src/platform-linux.cc (right): > > > > > > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode86 > > > src/platform-linux.cc:86: if (sizeof(void*) == 8 && (isolate = > > > Isolate::UncheckedCurrent())) { > > > I would prefer conditional compilation based on V8_TARGET_ARCH_X64 here > > instead > > > of sizeof(void*) comparison. > > > > > > Also we prefer explicit comparisons with NULL to implicit to boolean > > conversion: > > > > > > Isolate* isolate = Isolate::UncheckedCurrent(); > > > if (isolate != NULL) ... > > > > > > > http://codereview.chromium.org/7377008/diff/1/src/platform-linux.cc#newcode109 > > > src/platform-linux.cc:109: uint64_t seed = Ticks() | (getpid() << 16); > > > I think the comment above needs updating.
LGTM, I'll land it. |
