Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1677163003:
base: Create file mappings with reduced access control permissions. (Closed)
Descriptionbase: Create file mappings with reduced access control permissions.
A newly created file mapping has two sets of permissions. It has access control
permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file
permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the
parameter DUPLICATE_SAME_ACCESS copies both sets of permissions.
The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being
duplicated into unprivileged processes. But the only way to copy file
permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there
is no way for a privileged process to duplicate a file mapping into an
unprivileged process while maintaining the previous file permissions.
This CL removes all access control permissions of a file mapping immediately
after creation, which effectively means that ::DuplicateHandle() only copies
the file permissions.
These permissions are only enforced if the file mapping has a name, so this
CL also gives all file mappings a name.
BUG=493414
Committed: https://crrev.com/4b12c0a123b31adcccb095cb2a49ca7b59f9f1fb
Cr-Commit-Position: refs/heads/master@{#376358}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Fix type. #Patch Set 4 : Update comments. #Patch Set 5 : Always name shared memory handles. Add test. #Patch Set 6 : Update comments, again. #
Total comments: 6
Patch Set 7 : Comments from mark. #
Total comments: 2
Messages
Total messages: 51 (22 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677163003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677163003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677163003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677163003/40001
Description was changed from ========== test cl: lower permissions on creation of shm. BUG= ========== to ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. BUG=493414 ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Description was changed from ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. BUG=493414 ========== to ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. BUG=493414 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677163003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677163003/60001
erikchen@chromium.org changed reviewers: + tsepez@chromium.org
tsepez: Please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
tsepez@chromium.org changed reviewers: + wfh@chromium.org
LGTM, but get will or some windows expert to doublecheck.
tsepez@chromium.org changed reviewers: + pennymac@chromium.org - wfh@chromium.org
On 2016/02/10 00:30:54, Tom Sepez wrote: > LGTM, but get will or some windows expert to doublecheck. Will is OOO right now - do you want someone from the security team, or will anyone from the Windows team do?
On 2016/02/10 00:30:54, Tom Sepez wrote: > LGTM, but get will or some windows expert to doublecheck. Maybe Penny is virtual Will ...
On 2016/02/10 00:31:45, Tom Sepez wrote: > On 2016/02/10 00:30:54, Tom Sepez wrote: > > LGTM, but get will or some windows expert to doublecheck. > Maybe Penny is virtual Will ... LGTM, but you'll have to get base\OWNERS involved. Feel free to have jschuh@ do a sanity check as well - I wouldn't call myself a Chrome expert yet.
erikchen@chromium.org changed reviewers: + jschuh@chromium.org, mark@chromium.org
jschuh: Looking for a sanity check. mark: Looking for an OWNER review.
I have to think about what's correct here. The problem you're hitting is that Windows doesn't actually enforce permissions on anonymous mappings. So, nothing prevents the child from calling DuplicateHandle() again and adding the permissions back in. I'm not sure what the right solution is. Maybe we should just always name the object to get a proper permission set. Or maybe create a special exception in the hook for this case (because we probably don't care about the permissions). Lets chat tomorrow.
penny: PTAL I talked with jschuh, and he recommended always naming the object. I did so, and then wrote a test to verify that permissions cannot be added back by unprivileged processes.
Description was changed from ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. BUG=493414 ========== to ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. These permissions are only enforced if the file mapping has a name, so this CL also gives all file mappings a name. BUG=493414 ==========
The new test is great. See my tiny musings. Otherwise seems all good. https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_win.cc (right): https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... base/memory/shared_memory_win.cc:206: if (name_.empty()) { So, we're no longer looking at whether |share_read_only| is true or false in the SharedMemoryCreateOptions passed in. (We want to always name the mapping now, regardless.) 1) It appears the caller has always been expected to set the access controls to read-only when duplicating the handle later. In which case, |share_read_only| flag was only ever used to tell us to make sure that the caller COULD adjust the access controls later. So it no longer has a use. I can't immediately see anywhere else that this is looked at. If it's not referenced in other places, we could potentially remove that flag from source now. 2) SharedMemory::DuplicateHandle function does not seem to adjust anything for read-only either. Should we be enforcing read-only sharing somewhere in this file... or do callers need to do their own ::DuplicateHandle() system call with the appropriate accesses? I wonder why SharedMemory::DuplicateHandle doesn't support different access permissions for the new handle. jschuh@, thoughts?
https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_win.cc (right): https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... base/memory/shared_memory_win.cc:206: if (name_.empty()) { On 2016/02/17 00:32:24, penny wrote: > So, we're no longer looking at whether |share_read_only| is true or false in the > SharedMemoryCreateOptions passed in. (We want to always name the mapping now, > regardless.) > > 1) It appears the caller has always been expected to set the access controls to > read-only when duplicating the handle later. In which case, |share_read_only| > flag was only ever used to tell us to make sure that the caller COULD adjust the > access controls later. So it no longer has a use. I can't immediately see > anywhere else that this is looked at. > > If it's not referenced in other places, we could potentially remove that flag > from source now. > > 2) SharedMemory::DuplicateHandle function does not seem to adjust anything for > read-only either. Should we be enforcing read-only sharing somewhere in this > file... or do callers need to do their own ::DuplicateHandle() system call with > the appropriate accesses? I wonder why SharedMemory::DuplicateHandle doesn't > support different access permissions for the new handle. > > > jschuh@, thoughts? (1) is a correct assessment. I would prefer to remove |share_read_only| in a follow-up CL. And yes, the current system is very fragile, since a caller could forget to set up the handle correctly, and then attempt to duplicate read_only, and *think* that it had succeeded in making a read-only handle. (2) There are too many ways of duplicating SharedMemoryHandles on Windows. This is something I am working on fixing. (a) In this file, the method SharedMemory::ShareToProcessCommon() will duplicate a handle with read_only permissions, depending on the parameters it has been called with. (b) Windows sandbox code uses it's own logic where it calls ::DuplicateHandle() and manually sets the new permissions: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san... (c) There's also SharedMemory::DuplicateHandle, as you noticed, as well as other callers of ::DuplicateHandle (AttachmentBroker comes to mind, but there are probably others).
On 2016/02/17 00:43:22, erikchen wrote: > https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... > File base/memory/shared_memory_win.cc (right): > > https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... > base/memory/shared_memory_win.cc:206: if (name_.empty()) { > On 2016/02/17 00:32:24, penny wrote: > > So, we're no longer looking at whether |share_read_only| is true or false in > the > > SharedMemoryCreateOptions passed in. (We want to always name the mapping now, > > regardless.) > > > > 1) It appears the caller has always been expected to set the access controls > to > > read-only when duplicating the handle later. In which case, |share_read_only| > > flag was only ever used to tell us to make sure that the caller COULD adjust > the > > access controls later. So it no longer has a use. I can't immediately see > > anywhere else that this is looked at. > > > > If it's not referenced in other places, we could potentially remove that flag > > from source now. > > > > 2) SharedMemory::DuplicateHandle function does not seem to adjust anything for > > read-only either. Should we be enforcing read-only sharing somewhere in this > > file... or do callers need to do their own ::DuplicateHandle() system call > with > > the appropriate accesses? I wonder why SharedMemory::DuplicateHandle doesn't > > support different access permissions for the new handle. > > > > > > jschuh@, thoughts? > > (1) is a correct assessment. I would prefer to remove |share_read_only| in a > follow-up CL. And yes, the current system is very fragile, since a caller could > forget to set up the handle correctly, and then attempt to duplicate read_only, > and *think* that it had succeeded in making a read-only handle. > > (2) There are too many ways of duplicating SharedMemoryHandles on Windows. This > is something I am working on fixing. > > (a) In this file, the method SharedMemory::ShareToProcessCommon() will duplicate > a handle with read_only permissions, depending on the parameters it has been > called with. > > (b) Windows sandbox code uses it's own logic where it calls ::DuplicateHandle() > and manually sets the new permissions: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san... > > (c) There's also SharedMemory::DuplicateHandle, as you noticed, as well as other > callers of ::DuplicateHandle (AttachmentBroker comes to mind, but there are > probably others). LGTM 1) Removing |share_read_only| in a follow-up CL sounds nice and tidy. 2) Probably outside the scope of your small fix-up! Wouldn't worry about it given that |share_read_only| didn't have anything to do with enforcing read-only. :)
On 2016/02/17 00:47:39, penny wrote: > On 2016/02/17 00:43:22, erikchen wrote: > > > https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... > > File base/memory/shared_memory_win.cc (right): > > > > > https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory_win.cc:206: if (name_.empty()) { > > On 2016/02/17 00:32:24, penny wrote: > > > So, we're no longer looking at whether |share_read_only| is true or false in > > the > > > SharedMemoryCreateOptions passed in. (We want to always name the mapping > now, > > > regardless.) > > > > > > 1) It appears the caller has always been expected to set the access controls > > to > > > read-only when duplicating the handle later. In which case, > |share_read_only| > > > flag was only ever used to tell us to make sure that the caller COULD adjust > > the > > > access controls later. So it no longer has a use. I can't immediately see > > > anywhere else that this is looked at. > > > > > > If it's not referenced in other places, we could potentially remove that > flag > > > from source now. > > > > > > 2) SharedMemory::DuplicateHandle function does not seem to adjust anything > for > > > read-only either. Should we be enforcing read-only sharing somewhere in > this > > > file... or do callers need to do their own ::DuplicateHandle() system call > > with > > > the appropriate accesses? I wonder why SharedMemory::DuplicateHandle > doesn't > > > support different access permissions for the new handle. > > > > > > > > > jschuh@, thoughts? > > > > (1) is a correct assessment. I would prefer to remove |share_read_only| in a > > follow-up CL. And yes, the current system is very fragile, since a caller > could > > forget to set up the handle correctly, and then attempt to duplicate > read_only, > > and *think* that it had succeeded in making a read-only handle. > > > > (2) There are too many ways of duplicating SharedMemoryHandles on Windows. > This > > is something I am working on fixing. > > > > (a) In this file, the method SharedMemory::ShareToProcessCommon() will > duplicate > > a handle with read_only permissions, depending on the parameters it has been > > called with. > > > > (b) Windows sandbox code uses it's own logic where it calls > ::DuplicateHandle() > > and manually sets the new permissions: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san... > > > > (c) There's also SharedMemory::DuplicateHandle, as you noticed, as well as > other > > callers of ::DuplicateHandle (AttachmentBroker comes to mind, but there are > > probably others). > > LGTM > > 1) Removing |share_read_only| in a follow-up CL sounds nice and tidy. > 2) Probably outside the scope of your small fix-up! Wouldn't worry about it > given that |share_read_only| didn't have anything to do with enforcing > read-only. :) mark: PTAL
LGTM https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_win.cc (right): https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... base/memory/shared_memory_win.cc:95: ::CloseHandle(h); BOOL rv = ::CloseHandle(h); DCHECK(rv); ? https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_win_unittest.cc (right): https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... base/memory/shared_memory_win_unittest.cc:37: DuplicateTokenEx(process_token.Get(), 0, NULL, SecurityImpersonation, Windows Chrome style is to have a leading :: on win32 calls, like this one and ConvertStringSidToSid. (But it’s OK with me if you don’t do this, because I think it’s kind of silly.)
https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_win.cc (right): https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... base/memory/shared_memory_win.cc:95: ::CloseHandle(h); On 2016/02/17 19:03:14, Mark Mentovai wrote: > BOOL rv = ::CloseHandle(h); > DCHECK(rv); > > ? Done. https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_win_unittest.cc (right): https://codereview.chromium.org/1677163003/diff/100001/base/memory/shared_mem... base/memory/shared_memory_win_unittest.cc:37: DuplicateTokenEx(process_token.Get(), 0, NULL, SecurityImpersonation, On 2016/02/17 19:03:14, Mark Mentovai wrote: > Windows Chrome style is to have a leading :: on win32 calls, like this one and > ConvertStringSidToSid. (But it’s OK with me if you don’t do this, because I > think it’s kind of silly.) I went through the file and added many instances of "::".
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, pennymac@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/1677163003/#ps120001 (title: "Comments from mark.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677163003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677163003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677163003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677163003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/02/18 21:40:52, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) failures were because of more sandboxed processes trying to create SHMs. Will retry after this CL lands: https://codereview.chromium.org/1714643002/
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677163003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677163003/120001
Message was sent while issue was closed.
Description was changed from ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. These permissions are only enforced if the file mapping has a name, so this CL also gives all file mappings a name. BUG=493414 ========== to ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. These permissions are only enforced if the file mapping has a name, so this CL also gives all file mappings a name. BUG=493414 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. These permissions are only enforced if the file mapping has a name, so this CL also gives all file mappings a name. BUG=493414 ========== to ========== base: Create file mappings with reduced access control permissions. A newly created file mapping has two sets of permissions. It has access control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle() with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions. The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being duplicated into unprivileged processes. But the only way to copy file permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that there is no way for a privileged process to duplicate a file mapping into an unprivileged process while maintaining the previous file permissions. This CL removes all access control permissions of a file mapping immediately after creation, which effectively means that ::DuplicateHandle() only copies the file permissions. These permissions are only enforced if the file mapping has a name, so this CL also gives all file mappings a name. BUG=493414 Committed: https://crrev.com/4b12c0a123b31adcccb095cb2a49ca7b59f9f1fb Cr-Commit-Position: refs/heads/master@{#376358} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4b12c0a123b31adcccb095cb2a49ca7b59f9f1fb Cr-Commit-Position: refs/heads/master@{#376358}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1677163003/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_win_unittest.cc (right): https://codereview.chromium.org/1677163003/diff/120001/base/memory/shared_mem... base/memory/shared_memory_win_unittest.cc:50: TOKEN_MANDATORY_LABEL TIL = {0}; FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m32 /nologo /showIncludes /FC @obj\base\memory\base_unittests.shared_memory_win_unittest.obj.rsp /c ..\..\base\memory\shared_memory_win_unittest.cc /Foobj\base\memory\base_unittests.shared_memory_win_unittest.obj /Fdobj\base\base_unittests.cc.pdb ..\..\base\memory\shared_memory_win_unittest.cc(50,32) : error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] TOKEN_MANDATORY_LABEL TIL = {0}; ^ {} Please fix (just use {})
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1677163003/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_win_unittest.cc (right): https://codereview.chromium.org/1677163003/diff/120001/base/memory/shared_mem... base/memory/shared_memory_win_unittest.cc:87: uint32_t handle_as_int = reinterpret_cast<uint32_t>(handle); VC++ 2015 prefers double casts for the truncation-plus-type-change, best done with base::win::HandleToUint32. CL to do this (crrev.com/1715983002) and avoid the warning is about to be sent for review. This is just an FYI. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
