Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183833004:
Make chrome_elf use thunks instead of function pointers. (Closed)
|
Created:
6 years, 9 months ago by Cait (Slow) Modified:
6 years, 9 months ago CC:
chromium-reviews, caitkp+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake chrome_elf use thunks instead of function pointers.
1. Add functionality to ServiceResolverThunk to copy a thunk without patching.
2. Move chrome_elf thunk-handling code to a common location.
3. Use a thunk instead of a f'n ptr for redirects.
BUG=334379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255151
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257749
Patch Set 1 #
Total comments: 24
Patch Set 2 : Move all memory mgmt out of CopyThunk #
Total comments: 12
Patch Set 3 : Add size checks when copying thunk #Patch Set 4 : No bad thunks in the lookup table #
Total comments: 4
Patch Set 5 : Clean up documentation #Patch Set 6 : #Patch Set 7 : Use thunk instead of lookup table #
Total comments: 6
Patch Set 8 : Address comments, add scoped ptr #
Messages
Total messages: 39 (0 generated)
rvargas: PTAL at sandbox/ changes. I added a new public method to ServiceResolverThunk as we discussed (creatively named CopyThunk(), feel free to suggest other options) -- let me know if you'd like me to add test coverage for it. robertshield: PTAL at chrome_elf/ changes. Thanks!
https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... File sandbox/win/src/service_resolver_32.cc (right): https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:191: size_t thunk_bytes = GetThunkSize(); I'm fine with this code as is, but I would also be fine if the contract is to return only the service code (without the space to store the actual thunk (InternalThunk), given that there will be no interception). Just saying. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:201: BYTE* thunk_storage_bytes = reinterpret_cast<BYTE*>(thunk_storage); Why have the rest of this code here? We can assume that there's no need to copy this to the child (no?). As for the memory in this process, the caller should collect all the services into a single buffer and protect the whole thing at once.
https://codereview.chromium.org/183833004/diff/1/chrome_elf/ntdll_cache.cc File chrome_elf/ntdll_cache.cc (right): https://codereview.chromium.org/183833004/diff/1/chrome_elf/ntdll_cache.cc#ne... chrome_elf/ntdll_cache.cc:71: size_t storage_used; = 0 https://codereview.chromium.org/183833004/diff/1/chrome_elf/thunk_getter.cc File chrome_elf/thunk_getter.cc (right): https://codereview.chromium.org/183833004/diff/1/chrome_elf/thunk_getter.cc#n... chrome_elf/thunk_getter.cc:44: if (!(*is_wow64_process)(GetCurrentProcess(), &is_wow64)) can't you remove the parens and the dereference on the function pointer? https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... File sandbox/win/src/service_resolver_32.cc (right): https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:188: if(!NT_SUCCESS(ret)) space after if https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:201: BYTE* thunk_storage_bytes = reinterpret_cast<BYTE*>(thunk_storage); On 2014/02/28 19:40:09, rvargas wrote: > Why have the rest of this code here? We can assume that there's no need to copy > this to the child (no?). As for the memory in this process, the caller should > collect all the services into a single buffer and protect the whole thing at > once. I agree with this, I think the memory protection code can be moved out to the caller. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:206: if(!VirtualProtect(thunk_storage, space after if https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:214: SIZE_T written; = 0 https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:227: if (! VirtualProtect(thunk_storage, nit: no space after ! https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... File sandbox/win/src/service_resolver_64.cc (right): https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:125: if(!NT_SUCCESS(ret)) space after if https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:129: scoped_ptr<char[]> thunk_buffer(new char[thunk_bytes]); we use BYTE below for storage and chars here. Should be consistent. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:141: if(!VirtualProtect(thunk_storage, space after if https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:149: SIZE_T written; = 0
PTAL. CopyThunk now just verifies that the target f'n is a service, and copies the thunk into a buffer provided by the caller. Thanks! https://codereview.chromium.org/183833004/diff/1/chrome_elf/ntdll_cache.cc File chrome_elf/ntdll_cache.cc (right): https://codereview.chromium.org/183833004/diff/1/chrome_elf/ntdll_cache.cc#ne... chrome_elf/ntdll_cache.cc:71: size_t storage_used; On 2014/02/28 21:02:22, robertshield wrote: > = 0 Done. https://codereview.chromium.org/183833004/diff/1/chrome_elf/thunk_getter.cc File chrome_elf/thunk_getter.cc (right): https://codereview.chromium.org/183833004/diff/1/chrome_elf/thunk_getter.cc#n... chrome_elf/thunk_getter.cc:44: if (!(*is_wow64_process)(GetCurrentProcess(), &is_wow64)) On 2014/02/28 21:02:22, robertshield wrote: > can't you remove the parens and the dereference on the function pointer? Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... File sandbox/win/src/service_resolver_32.cc (right): https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:188: if(!NT_SUCCESS(ret)) On 2014/02/28 21:02:22, robertshield wrote: > space after if Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:201: BYTE* thunk_storage_bytes = reinterpret_cast<BYTE*>(thunk_storage); On 2014/02/28 21:02:22, robertshield wrote: > On 2014/02/28 19:40:09, rvargas wrote: > > Why have the rest of this code here? We can assume that there's no need to > copy > > this to the child (no?). As for the memory in this process, the caller should > > collect all the services into a single buffer and protect the whole thing at > > once. > > I agree with this, I think the memory protection code can be moved out to the > caller. Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:206: if(!VirtualProtect(thunk_storage, On 2014/02/28 21:02:22, robertshield wrote: > space after if Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:214: SIZE_T written; On 2014/02/28 21:02:22, robertshield wrote: > = 0 Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_32.cc:227: if (! VirtualProtect(thunk_storage, On 2014/02/28 21:02:22, robertshield wrote: > nit: no space after ! Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... File sandbox/win/src/service_resolver_64.cc (right): https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:125: if(!NT_SUCCESS(ret)) On 2014/02/28 21:02:22, robertshield wrote: > space after if Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:129: scoped_ptr<char[]> thunk_buffer(new char[thunk_bytes]); On 2014/02/28 21:02:22, robertshield wrote: > we use BYTE below for storage and chars here. Should be consistent. Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:141: if(!VirtualProtect(thunk_storage, On 2014/02/28 21:02:22, robertshield wrote: > space after if Done. https://codereview.chromium.org/183833004/diff/1/sandbox/win/src/service_reso... sandbox/win/src/service_resolver_64.cc:149: SIZE_T written; On 2014/02/28 21:02:22, robertshield wrote: > = 0 Done.
https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... File sandbox/win/src/service_resolver_32.cc (right): https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... sandbox/win/src/service_resolver_32.cc:191: size_t thunk_bytes = GetThunkSize(); Did you mean to compare this against storage_bytes? Shouldn't we update storage_used?
https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.cc File chrome_elf/ntdll_cache.cc (right): https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.c... chrome_elf/ntdll_cache.cc:68: if (!thunk) This implies that the rest of the function is optional, in which case I prefer the form if (thunk) { <code on lines 71-97> } in case someone attempts to add some non-optional code to the bottom of the function in the future. https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.c... chrome_elf/ntdll_cache.cc:76: if (!VirtualProtect(&g_nt_thunk_storage, In some places in this file we use the scope operator on API calls (see ::GetModuleHandle below) and in others we don't. We should be consistent. FWIW, I prefer with the scope operator. I feel it calls out Win32 API calls more clearly. https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.c... chrome_elf/ntdll_cache.cc:96: g_ntdll_lookup[std::string("NtCreateFile")] = Is the explicit string constructor needed? https://codereview.chromium.org/183833004/diff/40001/chrome_elf/thunk_getter.cc File chrome_elf/thunk_getter.cc (right): https://codereview.chromium.org/183833004/diff/40001/chrome_elf/thunk_getter.... chrome_elf/thunk_getter.cc:125: // Because Windows 8 and 8.1 have different stubs in 64-bit, Rereading this comment, I don't like the sentence structure (very possible that I wrote it :-)). Can we delete the part on line 125? https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... File sandbox/win/src/service_resolver_64.cc (right): https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... sandbox/win/src/service_resolver_64.cc:128: size_t thunk_bytes = GetThunkSize(); Please address Ricardo's note about size checking and returning |storage_used| here too.
thanks! https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.cc File chrome_elf/ntdll_cache.cc (right): https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.c... chrome_elf/ntdll_cache.cc:68: if (!thunk) On 2014/03/04 01:47:07, robertshield wrote: > This implies that the rest of the function is optional, in which case I prefer > the form > > if (thunk) { > <code on lines 71-97> > } > > in case someone attempts to add some non-optional code to the bottom of the > function in the future. Done. https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.c... chrome_elf/ntdll_cache.cc:76: if (!VirtualProtect(&g_nt_thunk_storage, On 2014/03/04 01:47:07, robertshield wrote: > In some places in this file we use the scope operator on API calls (see > ::GetModuleHandle below) and in others we don't. We should be consistent. > > FWIW, I prefer with the scope operator. I feel it calls out Win32 API calls more > clearly. Done. https://codereview.chromium.org/183833004/diff/40001/chrome_elf/ntdll_cache.c... chrome_elf/ntdll_cache.cc:96: g_ntdll_lookup[std::string("NtCreateFile")] = On 2014/03/04 01:47:07, robertshield wrote: > Is the explicit string constructor needed? Done. https://codereview.chromium.org/183833004/diff/40001/chrome_elf/thunk_getter.cc File chrome_elf/thunk_getter.cc (right): https://codereview.chromium.org/183833004/diff/40001/chrome_elf/thunk_getter.... chrome_elf/thunk_getter.cc:125: // Because Windows 8 and 8.1 have different stubs in 64-bit, On 2014/03/04 01:47:07, robertshield wrote: > Rereading this comment, I don't like the sentence structure (very possible that > I wrote it :-)). > > Can we delete the part on line 125? Done. https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... File sandbox/win/src/service_resolver_32.cc (right): https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... sandbox/win/src/service_resolver_32.cc:191: size_t thunk_bytes = GetThunkSize(); On 2014/03/03 22:36:46, rvargas wrote: > Did you mean to compare this against storage_bytes? Shouldn't we update > storage_used? Done. https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... File sandbox/win/src/service_resolver_64.cc (right): https://codereview.chromium.org/183833004/diff/40001/sandbox/win/src/service_... sandbox/win/src/service_resolver_64.cc:128: size_t thunk_bytes = GetThunkSize(); On 2014/03/04 01:47:07, robertshield wrote: > Please address Ricardo's note about size checking and returning |storage_used| > here too. Done.
lgtm https://codereview.chromium.org/183833004/diff/100001/chrome_elf/thunk_getter.h File chrome_elf/thunk_getter.h (right): https://codereview.chromium.org/183833004/diff/100001/chrome_elf/thunk_getter... chrome_elf/thunk_getter.h:12: sandbox::ServiceResolverThunk* GetThunk(bool relaxed); Mention that ownership is passed to the caller.
lgtm https://codereview.chromium.org/183833004/diff/100001/sandbox/win/src/service... File sandbox/win/src/service_resolver.h (right): https://codereview.chromium.org/183833004/diff/100001/sandbox/win/src/service... sandbox/win/src/service_resolver.h:50: // service and copy the first |storage_bytes| of data from that function into nit: I think the implementation is correct, but this comment is not completely aligned with reality, as the function fails if storage_bytes is too small (unfortunately, there's no public way of getting the minimum size). Maybe replace the last part with something like "and copies that function into |thunk_storage|" BTW, verify->verifies.
The CQ bit was checked by caitkp@chromium.org
Thanks for the reviews! https://codereview.chromium.org/183833004/diff/100001/chrome_elf/thunk_getter.h File chrome_elf/thunk_getter.h (right): https://codereview.chromium.org/183833004/diff/100001/chrome_elf/thunk_getter... chrome_elf/thunk_getter.h:12: sandbox::ServiceResolverThunk* GetThunk(bool relaxed); On 2014/03/04 15:57:42, robertshield wrote: > Mention that ownership is passed to the caller. Done. https://codereview.chromium.org/183833004/diff/100001/sandbox/win/src/service... File sandbox/win/src/service_resolver.h (right): https://codereview.chromium.org/183833004/diff/100001/sandbox/win/src/service... sandbox/win/src/service_resolver.h:50: // service and copy the first |storage_bytes| of data from that function into On 2014/03/04 19:11:35, rvargas wrote: > nit: I think the implementation is correct, but this comment is not completely > aligned with reality, as the function fails if storage_bytes is too small > (unfortunately, there's no public way of getting the minimum size). > > Maybe replace the last part with something like "and copies that function into > |thunk_storage|" > > BTW, verify->verifies. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome_elf/blacklist/blacklist.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome_elf/blacklist/blacklist.cc
Hunk #1 FAILED at 8.
Hunk #2 FAILED at 41.
Hunk #3 succeeded at 361 (offset 32 lines).
Hunk #4 succeeded at 395 (offset 32 lines).
2 out of 4 hunks FAILED -- saving rejects to file
chrome_elf/blacklist/blacklist.cc.rej
Patch: chrome_elf/blacklist/blacklist.cc
Index: chrome_elf/blacklist/blacklist.cc
diff --git a/chrome_elf/blacklist/blacklist.cc
b/chrome_elf/blacklist/blacklist.cc
index
958d7103b798ba5488b8d9857f7be6a06c20cecb..9796c96b7a382199fb1647a8395f5c8416269555
100644
--- a/chrome_elf/blacklist/blacklist.cc
+++ b/chrome_elf/blacklist/blacklist.cc
@@ -8,9 +8,9 @@
#include "base/basictypes.h"
#include "chrome_elf/blacklist/blacklist_interceptions.h"
+#include "chrome_elf/thunk_getter.h"
#include "sandbox/win/src/interception_internal.h"
#include "sandbox/win/src/internal_types.h"
-#include "sandbox/win/src/sandbox_utils.h"
#include "sandbox/win/src/service_resolver.h"
#include "version.h" // NOLINT
@@ -41,109 +41,10 @@ __declspec(allocate(".crthunk")) sandbox::ThunkData
g_thunk_storage;
namespace {
-enum Version {
- VERSION_PRE_XP_SP2 = 0, // Not supported.
- VERSION_XP_SP2,
- VERSION_SERVER_2003, // Also includes XP Pro x64 and Server 2003 R2.
- VERSION_VISTA, // Also includes Windows Server 2008.
- VERSION_WIN7, // Also includes Windows Server 2008 R2.
- VERSION_WIN8, // Also includes Windows Server 2012.
- VERSION_WIN8_1,
- VERSION_WIN_LAST, // Indicates error condition.
-};
-
-// Whether a process is running under WOW64 (the wrapper that allows 32-bit
-// processes to run on 64-bit versions of Windows). This will return
-// WOW64_DISABLED for both "32-bit Chrome on 32-bit Windows" and "64-bit
-// Chrome on 64-bit Windows". WOW64_UNKNOWN means "an error occurred", e.g.
-// the process does not have sufficient access rights to determine this.
-enum WOW64Status {
- WOW64_DISABLED,
- WOW64_ENABLED,
- WOW64_UNKNOWN,
-};
-
// Record if the blacklist was successfully initialized so processes can easily
// determine if the blacklist is enabled for them.
bool g_blacklist_initialized = false;
-WOW64Status GetWOW64StatusForCurrentProcess() {
- typedef BOOL (WINAPI* IsWow64ProcessFunc)(HANDLE, PBOOL);
- IsWow64ProcessFunc is_wow64_process = reinterpret_cast<IsWow64ProcessFunc>(
- GetProcAddress(GetModuleHandle(L"kernel32.dll"), "IsWow64Process"));
- if (!is_wow64_process)
- return WOW64_DISABLED;
- BOOL is_wow64 = FALSE;
- if (!(*is_wow64_process)(GetCurrentProcess(), &is_wow64))
- return WOW64_UNKNOWN;
- return is_wow64 ? WOW64_ENABLED : WOW64_DISABLED;
-}
-
-class OSInfo {
- public:
- struct VersionNumber {
- int major;
- int minor;
- int build;
- };
-
- struct ServicePack {
- int major;
- int minor;
- };
-
- OSInfo() {
- OSVERSIONINFOEX version_info = { sizeof(version_info) };
- GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info));
- version_number_.major = version_info.dwMajorVersion;
- version_number_.minor = version_info.dwMinorVersion;
- version_number_.build = version_info.dwBuildNumber;
- if ((version_number_.major == 5) && (version_number_.minor > 0)) {
- // Treat XP Pro x64, Home Server, and Server 2003 R2 as Server 2003.
- version_ = (version_number_.minor == 1) ? VERSION_XP_SP2 :
- VERSION_SERVER_2003;
- if (version_ == VERSION_XP_SP2 && version_info.wServicePackMajor < 2)
- version_ = VERSION_PRE_XP_SP2;
- } else if (version_number_.major == 6) {
- switch (version_number_.minor) {
- case 0:
- // Treat Windows Server 2008 the same as Windows Vista.
- version_ = VERSION_VISTA;
- break;
- case 1:
- // Treat Windows Server 2008 R2 the same as Windows 7.
- version_ = VERSION_WIN7;
- break;
- case 2:
- // Treat Windows Server 2012 the same as Windows 8.
- version_ = VERSION_WIN8;
- break;
- default:
- version_ = VERSION_WIN8_1;
- break;
- }
- } else if (version_number_.major > 6) {
- version_ = VERSION_WIN_LAST;
- } else {
- version_ = VERSION_PRE_XP_SP2;
- }
-
- service_pack_.major = version_info.wServicePackMajor;
- service_pack_.minor = version_info.wServicePackMinor;
- }
-
- Version version() const { return version_; }
- VersionNumber version_number() const { return version_number_; }
- ServicePack service_pack() const { return service_pack_; }
-
- private:
- Version version_;
- VersionNumber version_number_;
- ServicePack service_pack_;
-
- DISALLOW_COPY_AND_ASSIGN(OSInfo);
-};
-
bool IsNonBrowserProcess() {
typedef bool (*IsSandboxedProcessFunc)();
IsSandboxedProcessFunc is_sandboxed_process =
@@ -329,17 +230,16 @@ bool Initialize(bool force) {
if (!force && !LeaveSetupBeacon())
return false;
- // Don't try blacklisting on unsupported OS versions.
- OSInfo os_info;
- if (os_info.version() <= VERSION_PRE_XP_SP2)
- return false;
-
- // Pseudo-handle, no need to close.
- HANDLE current_process = ::GetCurrentProcess();
-
// Tells the resolver to patch already patched functions.
const bool kRelaxed = true;
+ // Create a thunk via the appropriate ServiceResolver instance.
+ sandbox::ServiceResolverThunk* thunk = GetThunk(kRelaxed);
+
+ // Don't try blacklisting on unsupported OS versions.
+ if (!thunk)
+ return false;
+
// Record that we are starting the thunk setup code.
HKEY key = NULL;
DWORD disposition = 0;
@@ -364,26 +264,6 @@ bool Initialize(bool force) {
key = NULL;
}
- // Create a thunk via the appropriate ServiceResolver instance.
- sandbox::ServiceResolverThunk* thunk = NULL;
-#if defined(_WIN64)
- // Because Windows 8 and 8.1 have different stubs in 64-bit,
- // ServiceResolverThunk can handle all the formats in 64-bit (instead only
- // handling 1 like it does in 32-bit versions).
- thunk = new sandbox::ServiceResolverThunk(current_process, kRelaxed);
-#else
- if (GetWOW64StatusForCurrentProcess() == WOW64_ENABLED) {
- if (os_info.version() >= VERSION_WIN8)
- thunk = new sandbox::Wow64W8ResolverThunk(current_process, kRelaxed);
- else
- thunk = new sandbox::Wow64ResolverThunk(current_process, kRelaxed);
- } else if (os_info.version() >= VERSION_WIN8) {
- thunk = new sandbox::Win8ResolverThunk(current_process, kRelaxed);
- } else {
- thunk = new sandbox::ServiceResolverThunk(current_process, kRelaxed);
- }
-#endif
-
// Record that we have initialized the blacklist.
g_blacklist_initialized = true;
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/140001
Message was sent while issue was closed.
Change committed as 255151
Robert: PTAL -- here is a first pass at the changes we discussed. Thanks!
https://codereview.chromium.org/183833004/diff/160001/chrome_elf/create_file/... File chrome_elf/create_file/chrome_create_file.cc (right): https://codereview.chromium.org/183833004/diff/160001/chrome_elf/create_file/... chrome_elf/create_file/chrome_create_file.cc:192: char thunk_buffer[sizeof(sandbox::ThunkData)] = {}; Please add a comment that explains that this is for debugging purposes. https://codereview.chromium.org/183833004/diff/160001/chrome_elf/ntdll_cache.cc File chrome_elf/ntdll_cache.cc (right): https://codereview.chromium.org/183833004/diff/160001/chrome_elf/ntdll_cache.... chrome_elf/ntdll_cache.cc:67: sandbox::ServiceResolverThunk* thunk = GetThunk(kRelaxed); try using scoped_ptr if we can do so with dependencies. https://codereview.chromium.org/183833004/diff/160001/chrome_elf/ntdll_cache.... chrome_elf/ntdll_cache.cc:72: // Mark the thunk storage as readable and writeable, since we *we are
https://codereview.chromium.org/183833004/diff/160001/chrome_elf/create_file/... File chrome_elf/create_file/chrome_create_file.cc (right): https://codereview.chromium.org/183833004/diff/160001/chrome_elf/create_file/... chrome_elf/create_file/chrome_create_file.cc:192: char thunk_buffer[sizeof(sandbox::ThunkData)] = {}; On 2014/03/17 22:21:27, robertshield wrote: > Please add a comment that explains that this is for debugging purposes. Done. https://codereview.chromium.org/183833004/diff/160001/chrome_elf/ntdll_cache.cc File chrome_elf/ntdll_cache.cc (right): https://codereview.chromium.org/183833004/diff/160001/chrome_elf/ntdll_cache.... chrome_elf/ntdll_cache.cc:67: sandbox::ServiceResolverThunk* thunk = GetThunk(kRelaxed); On 2014/03/17 22:21:27, robertshield wrote: > try using scoped_ptr if we can do so with dependencies. Done. https://codereview.chromium.org/183833004/diff/160001/chrome_elf/ntdll_cache.... chrome_elf/ntdll_cache.cc:72: // Mark the thunk storage as readable and writeable, since we On 2014/03/17 22:21:27, robertshield wrote: > *we are Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/183833004/200001
Message was sent while issue was closed.
Change committed as 257749 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
