Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1798203002:
Support read/write memory-mapped files. (Closed)
|
Created:
4 years, 9 months ago by bcwhite Modified:
3 years, 6 months ago CC:
chromium-reviews, jshin+watch_chromium.org, vmpstr+watch_chromium.org, Lei Zhang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport read/write memory-mapped files.
See also: chromium-dev thread "Support for Writable Memory Mapped Files", specifically the last few messages starting with "And... we're back!".
BUG=546019
Committed: https://crrev.com/f0dfab0bef147f55d6460b42878ef71d0578cc59
Cr-Commit-Position: refs/heads/master@{#394868}
Patch Set 1 #Patch Set 2 : a bit of clean-up #Patch Set 3 : rebased #Patch Set 4 : fixed file extension on POSIX builds #Patch Set 5 : added READ_ONLY parameter to other initializations of MemoryMappedFile #Patch Set 6 : allow default READ_ONLY for all forms of Initialize() to avoid tiny refactoring in many places #Patch Set 7 : rebased #
Total comments: 14
Patch Set 8 : addressed comments by danakj #Patch Set 9 : rebased #
Total comments: 9
Patch Set 10 : removed Windows-specific InitializeAsImageSection call #Patch Set 11 : rebased #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 60 (30 generated)
bcwhite@chromium.org changed reviewers: + thestig@chromium.org
This isn't finished but I would like to get some feedback on the design & implementation before I go through the rest of the code-base to fix any broken instantiations. base_unittests compiles and runs successfully on Windows.
Description was changed from ========== Support read/write memory-mapped files. BUG=546019 ========== to ========== Support read/write memory-mapped files. BUG=546019 ==========
bcwhite@chromium.org changed reviewers: + mark@chromium.org - thestig@chromium.org
Description was changed from ========== Support read/write memory-mapped files. BUG=546019 ========== to ========== Support read/write memory-mapped files. See also: chromium-dev thread "Support for Writable Memory Mapped Files", specifically the last few messages starting with "And... we're back!". BUG=546019 ==========
On 2016/03/14 21:46:45, bcwhite wrote: > This isn't finished but I would like to get some feedback on the design & > implementation before I go through the rest of the code-base to fix any broken > instantiations. > > base_unittests compiles and runs successfully on Windows. -thestig (paternity leave) +mark
On 2016/03/16 21:40:22, bcwhite wrote: > On 2016/03/14 21:46:45, bcwhite wrote: > > This isn't finished but I would like to get some feedback on the design & > > implementation before I go through the rest of the code-base to fix any broken > > instantiations. > > > > base_unittests compiles and runs successfully on Windows. > > -thestig (paternity leave) > +mark Ping?
mark@chromium.org changed reviewers: + danakj@chromium.org
I’m at a summit this week and am a little constrained. +danakj for maybe quicker feedback.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
On 2016/03/30 19:46:23, Mark Mentovai wrote: > I’m at a summit this week and am a little constrained. +danakj for maybe quicker > feedback. Ping?
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #7 (id:270006) has been deleted
Patchset #7 (id:330001) has been deleted
Patchset #7 (id:350001) has been deleted
Patchset #7 (id:370001) has been deleted
On 2016/04/06 15:49:00, bcwhite wrote: > On 2016/03/30 19:46:23, Mark Mentovai wrote: > > I’m at a summit this week and am a little constrained. +danakj for maybe > quicker > > feedback. > > Ping? It's been a month, now. Could somebody review this, please.
Patchset #7 (id:390001) has been deleted
Patchset #7 (id:410001) has been deleted
Patchset #7 (id:430001) has been deleted
danakj@chromium.org changed reviewers: + thestig@chromium.org
+thestig who was active on the thread proposing this on chromium-dev His last comment was that this shouldn't be added in base. I don't see anything wrong with the implementation as written. I am also pretty hesitant about adding this capability to this class, as it will end up being used in lots of places, and it seems controversial at best. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file.h:67: // Opens an existing file and maps it into memory. |Access| can be read-only |access| should refer to the variable name (no capital) https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file.h:77: // As above, but works with an already-opened file. |Access| can be read-only |access| https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file.h:80: // permissions suitable for |access|. if not, false is returned? https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:81: file_.SetLength(std::max(file_.GetLength(), region.offset + region.size)); possible integer overflow, use checked math https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_unittest.cc:181: ASSERT_TRUE(map.data() != NULL); nullptr https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_unittest.cc:217: ASSERT_TRUE(map.data() != NULL); nullptr https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:47: size_low = static_cast<uint32_t>(region.size); this is technically undefined behaviour if size_high was non-zero. http://stackoverflow.com/questions/2241897/what-does-the-c-language-standard-... region.size & 0xffffffff?
> I don't see anything wrong with the implementation as > written. I am also pretty hesitant about adding this > capability to this class, as it will end up being > used in lots of places, and it seems controversial at > best. If somebody wants to do this, they'll do it. It was already suggested to me to just "roll your own". Doing that, however, means duplicated code that will generally not be as well tested, especially if it has to be cross-platform. Better, in my opinion, to have it common and well tested with clear warnings about the potential pitfalls of using it. Also, while there are concerns over how this is used in user-facing code, there are many other places that can benefit from this, testing for example. The SyzyASAN team wants to be able to use this to persist metrics for reporting from some Canary builds. Other alternatives involve much more code and tedious plumbing. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file.h:67: // Opens an existing file and maps it into memory. |Access| can be read-only On 2016/04/25 21:26:17, danakj wrote: > |access| should refer to the variable name (no capital) Done. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file.h:77: // As above, but works with an already-opened file. |Access| can be read-only On 2016/04/25 21:26:17, danakj wrote: > |access| Done. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file.h:80: // permissions suitable for |access|. On 2016/04/25 21:26:17, danakj wrote: > if not, false is returned? Done. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:81: file_.SetLength(std::max(file_.GetLength(), region.offset + region.size)); On 2016/04/25 21:26:17, danakj wrote: > possible integer overflow, use checked math Done for all platforms in memory_mapped_file.cc. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_unittest.cc:181: ASSERT_TRUE(map.data() != NULL); On 2016/04/25 21:26:17, danakj wrote: > nullptr Done here and elsewhere in file. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_unittest.cc:217: ASSERT_TRUE(map.data() != NULL); On 2016/04/25 21:26:17, danakj wrote: > nullptr Done. https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:47: size_low = static_cast<uint32_t>(region.size); On 2016/04/25 21:26:17, danakj wrote: > this is technically undefined behaviour if size_high was non-zero. > > http://stackoverflow.com/questions/2241897/what-does-the-c-language-standard-... > > region.size & 0xffffffff? Done.
On 2016/04/26 00:08:46, bcwhite wrote: > > I don't see anything wrong with the implementation as > > written. I am also pretty hesitant about adding this > > capability to this class, as it will end up being > > used in lots of places, and it seems controversial at > > best. > > If somebody wants to do this, they'll do it. It was already suggested to me to > just "roll your own". Doing that, however, means duplicated code that will > generally not be as well tested, especially if it has to be cross-platform. > > Better, in my opinion, to have it common and well tested with clear warnings > about the potential pitfalls of using it. > > Also, while there are concerns over how this is used in user-facing code, there > are many other places that can benefit from this, testing for example. The > SyzyASAN team wants to be able to use this to persist metrics for reporting from > some Canary builds. Other alternatives involve much more code and tedious > plumbing. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > File base/files/memory_mapped_file.h (right): > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file.h:67: // Opens an existing file and maps it into > memory. |Access| can be read-only > On 2016/04/25 21:26:17, danakj wrote: > > |access| should refer to the variable name (no capital) > > Done. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file.h:77: // As above, but works with an > already-opened file. |Access| can be read-only > On 2016/04/25 21:26:17, danakj wrote: > > |access| > > Done. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file.h:80: // permissions suitable for |access|. > On 2016/04/25 21:26:17, danakj wrote: > > if not, false is returned? > > Done. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > File base/files/memory_mapped_file_posix.cc (right): > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file_posix.cc:81: > file_.SetLength(std::max(file_.GetLength(), region.offset + region.size)); > On 2016/04/25 21:26:17, danakj wrote: > > possible integer overflow, use checked math > > Done for all platforms in memory_mapped_file.cc. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > File base/files/memory_mapped_file_unittest.cc (right): > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file_unittest.cc:181: ASSERT_TRUE(map.data() != NULL); > On 2016/04/25 21:26:17, danakj wrote: > > nullptr > > Done here and elsewhere in file. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file_unittest.cc:217: ASSERT_TRUE(map.data() != NULL); > On 2016/04/25 21:26:17, danakj wrote: > > nullptr > > Done. > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > File base/files/memory_mapped_file_win.cc (right): > > https://codereview.chromium.org/1798203002/diff/450001/base/files/memory_mapp... > base/files/memory_mapped_file_win.cc:47: size_low = > static_cast<uint32_t>(region.size); > On 2016/04/25 21:26:17, danakj wrote: > > this is technically undefined behaviour if size_high was non-zero. > > > > > http://stackoverflow.com/questions/2241897/what-does-the-c-language-standard-... > > > > region.size & 0xffffffff? > > Done. Ping?
After a bunch of summits, convergences, travel, and vacation, I’m ready to look at this myself (and stop hoping that Dana would deal with this or that Lei would return from leave). I’m very sorry for the delay, Brian. The code here is in fine shape. I read most of the 2015 discussion in the linked thread, and all of the 2016 discussion. I understand that there were some concerns about providing this as a general-purpose utility, but I also understand that there are reasons that this approach might be attractive. Although I’m sure that some of the concerns won’t be a problem in practice, I’m also mindful of the fact that Chrome has taken a pretty harsh stance against even the possibility of blocking I/O on arbitrary threads at times beyond our control. One question I have is what advantages there are for hiding (eventual) disk operations behind memory operations, relative to sharing a file handle or descriptor and making the disk operations explicit in a way that Chrome’s I/O-assertion machinery can detect. If we proceed with this, we should probably have someone from chrome-security think about it. I’d certainly want to tick that box before we start using this code.
On 2016/05/02 20:26:00, Mark Mentovai wrote: > After a bunch of summits, convergences, travel, and vacation, I’m ready to look > at this myself (and stop hoping that Dana would deal with this or that Lei would > return from leave). I’m very sorry for the delay, Brian. Lei is on paternity leave. I originally gave the review to him because he was involved in the original discussion. He suggested I pass it to you. Lucky you... ;-) > I read most of the 2015 discussion in the linked thread, and all of the 2016 > discussion. I understand that there were some concerns about providing this as a > general-purpose utility, but I also understand that there are reasons that this > approach might be attractive. Although I’m sure that some of the concerns won’t > be a problem in practice, I’m also mindful of the fact that Chrome has taken a > pretty harsh stance against even the possibility of blocking I/O on arbitrary > threads at times beyond our control. Definitely an issue. It's already an issue with the existing read-only code. It's used in many places, possibly without thought towards the potential issues listed in the new comments. But while mmap'd files will block on access (read or write) if the memory has not previously been paged in, there should(*) be no additional stalls just because it's a write operation. The OS will write out the dirty page on its own time without impacting the thread writing the memory. In this regard, it's no different than the OS paging out RAM when there is memory pressure. (*) I say "should" because, of course, the OS can choose to do whatever it wishes. > One question I have is what advantages there are for hiding (eventual) disk > operations behind memory operations, relative to sharing a file handle or > descriptor and making the disk operations explicit in a way that Chrome’s > I/O-assertion machinery can detect. If you're doing block reads or writes, direct I/O read/write calls are as easy as anything. But if you're doing memory operations (such as read-modify-write) then direct memory access is much, much more efficient. In my case, working on histograms, only direct memory access provides the kind of performance necessary and works without regard to threads and mutexes. Atomic memory ops provide all the protection I need. SyzyASAN wants to use this to provide backing for the ASAN metrics in Canary in a way that is easy to share with the main browser process and can survive a crash/hang without complex plumbing. > If we proceed with this, we should probably have someone from chrome-security > think about it. I’d certainly want to tick that box before we start using this > code. Sure. I'm happy to update this so that there's reason to expect that it's used in a safe manner, both performance-wise and security-wise. I think it has to be a balance, though. Make it too difficult and people will just work around limitations because "their case is different and should be allowed", potentially causing even more problems.
Anything new?
Yes. I think we should move forward with this. As you pointed out, I’d rather have a tested, working, security-reviewed version of this with the right warnings and usage guides hanging off of it than have people quietly roll their own whenever they need it. The fact that we use file-backed shared memory as read-only right now does kind of indicate that the worst and scariest bridges have already been crossed. We’re not going to back away from that, and I don’t see the point in making you contort your use case to use more obvious file I/O if the existing metrics code isn’t a good fit for that design. I think that we should mostly focus on getting someone from chrome-security to look at this to make sure that all of the gaps have been sealed. Bottom line: I’ll support this in base.
forshaw@chromium.org changed reviewers: + forshaw@chromium.org
https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; While we're changing this I'd highly recommend we remove support for image sections. As far as I can tell it's not used at present, is a Windows specific addition and leads to security issues like https://bugs.chromium.org/p/chromium/issues/detail?id=564238.
https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; On 2016/05/09 21:23:19, forshaw wrote: > While we're changing this I'd highly recommend we remove support for image > sections. As far as I can tell it's not used at present, is a Windows specific > addition and leads to security issues like > https://bugs.chromium.org/p/chromium/issues/detail?id=564238. Hmmm.... How about I work towards that in a different CL so as to not hold up this one unnecessarily?
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file.cc:47: NOTREACHED(); Should this be a CHECK(false) instead? https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:52: size_high, size_low, NULL)); A question, mostly for forshaw: Should we be removing the access control permissions, and also giving the FileMapping a name, as in https://codereview.chromium.org/1677163003
https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; On 2016/05/10 00:44:41, bcwhite wrote: > On 2016/05/09 21:23:19, forshaw wrote: > > While we're changing this I'd highly recommend we remove support for image > > sections. As far as I can tell it's not used at present, is a Windows specific > > addition and leads to security issues like > > https://bugs.chromium.org/p/chromium/issues/detail?id=564238. > > Hmmm.... How about I work towards that in a different CL so as to not hold up > this one unnecessarily? Well we could do it in another CL, sure. I'd argue that especially in light of the changes being made the support for image sections doesn't make any sense. You can't have a "writable" image section, at the least passing PAGE_READONLY or PAGE_READWRITE with SEC_IMAGE makes no difference to what actually gets mapped. If I was being pedantic you could argue that the code should have some sort of check (even a DCHECK) that ensures image section and writable are not being used at the same time, but if we do that change it might just be simpler and reduce code to just remove support (assuming no one else is using it, after all there's not even a unit test for the image section code). https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:52: size_high, size_low, NULL)); On 2016/05/10 02:21:51, erikchen wrote: > A question, mostly for forshaw: > > Should we be removing the access control permissions, and also giving the > FileMapping a name, as in https://codereview.chromium.org/1677163003 I don't believe so, at least as far as I can tell there's no way of sharing a memory mapped file, it's only for the purposes of mapping a file into the current process. Certainly you could share the File object to another process but the transport of PlatformFile instances should already be handled. Once at the other side the section object would be created anew. We do hold a reference to the file_mapping handle, but there's no method to extract it for transport. This code also doesn't need to maintain that reference, it would be possible to close the handle after calling MapViewOfFile and nothing would go wrong.
https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file.cc:47: NOTREACHED(); On 2016/05/10 02:21:51, erikchen wrote: > Should this be a CHECK(false) instead? Only if you think we're going to ship code that does this.
https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file.cc:47: NOTREACHED(); On 2016/05/10 02:21:51, erikchen wrote: > Should this be a CHECK(false) instead? Not really necessary. A non-DCHECK build would get flags=0 and fail in Initialize() below. https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; On 2016/05/10 07:04:30, forshaw wrote: > On 2016/05/10 00:44:41, bcwhite wrote: > > On 2016/05/09 21:23:19, forshaw wrote: > > > While we're changing this I'd highly recommend we remove support for image > > > sections. As far as I can tell it's not used at present, is a Windows > specific > > > addition and leads to security issues like > > > https://bugs.chromium.org/p/chromium/issues/detail?id=564238. > > > > Hmmm.... How about I work towards that in a different CL so as to not hold up > > this one unnecessarily? > > Well we could do it in another CL, sure. I'd argue that especially in light of > the changes being made the support for image sections doesn't make any sense. > You can't have a "writable" image section, at the least passing PAGE_READONLY or > PAGE_READWRITE with SEC_IMAGE makes no difference to what actually gets mapped. > If I was being pedantic you could argue that the code should have some sort of > check (even a DCHECK) that ensures image section and writable are not being used > at the same time, but if we do that change it might just be simpler and reduce > code to just remove support (assuming no one else is using it, after all there's > not even a unit test for the image section code). Since it's not actually in-use anywhere, I guess there's no delay by removing it now. Done.
On 2016/05/10 21:00:54, bcwhite wrote: > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > File base/files/memory_mapped_file.cc (right): > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > base/files/memory_mapped_file.cc:47: NOTREACHED(); > On 2016/05/10 02:21:51, erikchen wrote: > > Should this be a CHECK(false) instead? > > Not really necessary. A non-DCHECK build would get flags=0 and fail in > Initialize() below. > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > File base/files/memory_mapped_file_win.cc (right): > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; > On 2016/05/10 07:04:30, forshaw wrote: > > On 2016/05/10 00:44:41, bcwhite wrote: > > > On 2016/05/09 21:23:19, forshaw wrote: > > > > While we're changing this I'd highly recommend we remove support for image > > > > sections. As far as I can tell it's not used at present, is a Windows > > specific > > > > addition and leads to security issues like > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=564238. > > > > > > Hmmm.... How about I work towards that in a different CL so as to not hold > up > > > this one unnecessarily? > > > > Well we could do it in another CL, sure. I'd argue that especially in light of > > the changes being made the support for image sections doesn't make any sense. > > You can't have a "writable" image section, at the least passing PAGE_READONLY > or > > PAGE_READWRITE with SEC_IMAGE makes no difference to what actually gets > mapped. > > If I was being pedantic you could argue that the code should have some sort of > > check (even a DCHECK) that ensures image section and writable are not being > used > > at the same time, but if we do that change it might just be simpler and reduce > > code to just remove support (assuming no one else is using it, after all > there's > > not even a unit test for the image section code). > > Since it's not actually in-use anywhere, I guess there's no delay by removing it > now. Done. Ping?
On 2016/05/18 15:23:13, bcwhite wrote: > On 2016/05/10 21:00:54, bcwhite wrote: > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > File base/files/memory_mapped_file.cc (right): > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > base/files/memory_mapped_file.cc:47: NOTREACHED(); > > On 2016/05/10 02:21:51, erikchen wrote: > > > Should this be a CHECK(false) instead? > > > > Not really necessary. A non-DCHECK build would get flags=0 and fail in > > Initialize() below. > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > File base/files/memory_mapped_file_win.cc (right): > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; > > On 2016/05/10 07:04:30, forshaw wrote: > > > On 2016/05/10 00:44:41, bcwhite wrote: > > > > On 2016/05/09 21:23:19, forshaw wrote: > > > > > While we're changing this I'd highly recommend we remove support for > image > > > > > sections. As far as I can tell it's not used at present, is a Windows > > > specific > > > > > addition and leads to security issues like > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=564238. > > > > > > > > Hmmm.... How about I work towards that in a different CL so as to not > hold > > up > > > > this one unnecessarily? > > > > > > Well we could do it in another CL, sure. I'd argue that especially in light > of > > > the changes being made the support for image sections doesn't make any > sense. > > > You can't have a "writable" image section, at the least passing > PAGE_READONLY > > or > > > PAGE_READWRITE with SEC_IMAGE makes no difference to what actually gets > > mapped. > > > If I was being pedantic you could argue that the code should have some sort > of > > > check (even a DCHECK) that ensures image section and writable are not being > > used > > > at the same time, but if we do that change it might just be simpler and > reduce > > > code to just remove support (assuming no one else is using it, after all > > there's > > > not even a unit test for the image section code). > > > > Since it's not actually in-use anywhere, I guess there's no delay by removing > it > > now. Done. > > Ping? lgtm.
On 2016/05/18 17:45:07, forshaw wrote: > On 2016/05/18 15:23:13, bcwhite wrote: > > On 2016/05/10 21:00:54, bcwhite wrote: > > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > > File base/files/memory_mapped_file.cc (right): > > > > > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > > base/files/memory_mapped_file.cc:47: NOTREACHED(); > > > On 2016/05/10 02:21:51, erikchen wrote: > > > > Should this be a CHECK(false) instead? > > > > > > Not really necessary. A non-DCHECK build would get flags=0 and fail in > > > Initialize() below. > > > > > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > > File base/files/memory_mapped_file_win.cc (right): > > > > > > > > > https://codereview.chromium.org/1798203002/diff/490001/base/files/memory_mapp... > > > base/files/memory_mapped_file_win.cc:34: int flags = image_ ? SEC_IMAGE : 0; > > > On 2016/05/10 07:04:30, forshaw wrote: > > > > On 2016/05/10 00:44:41, bcwhite wrote: > > > > > On 2016/05/09 21:23:19, forshaw wrote: > > > > > > While we're changing this I'd highly recommend we remove support for > > image > > > > > > sections. As far as I can tell it's not used at present, is a Windows > > > > specific > > > > > > addition and leads to security issues like > > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=564238. > > > > > > > > > > Hmmm.... How about I work towards that in a different CL so as to not > > hold > > > up > > > > > this one unnecessarily? > > > > > > > > Well we could do it in another CL, sure. I'd argue that especially in > light > > of > > > > the changes being made the support for image sections doesn't make any > > sense. > > > > You can't have a "writable" image section, at the least passing > > PAGE_READONLY > > > or > > > > PAGE_READWRITE with SEC_IMAGE makes no difference to what actually gets > > > mapped. > > > > If I was being pedantic you could argue that the code should have some > sort > > of > > > > check (even a DCHECK) that ensures image section and writable are not > being > > > used > > > > at the same time, but if we do that change it might just be simpler and > > reduce > > > > code to just remove support (assuming no one else is using it, after all > > > there's > > > > not even a unit test for the image section code). > > > > > > Since it's not actually in-use anywhere, I guess there's no delay by > removing > > it > > > now. Done. > > > > Ping? > > lgtm. Could I get the same from an owner, please?
Yes. One more question. https://codereview.chromium.org/1798203002/diff/530001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/1798203002/diff/530001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:78: // POSIX won't auto-extend the file when it is written so it must first What kinds of sizes are we talking about here? HFS+, the OS X filesystem, doesn’t support sparse files, so all of these zeroes will actually be written to disk.
https://codereview.chromium.org/1798203002/diff/530001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/1798203002/diff/530001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:78: // POSIX won't auto-extend the file when it is written so it must first On 2016/05/19 14:57:06, Mark Mentovai wrote: > What kinds of sizes are we talking about here? HFS+, the OS X filesystem, > doesn’t support sparse files, so all of these zeroes will actually be written to > disk. That's annoying. For my immediate use (sharing SyzyASAN metrics with the browser on some fraction of Canary users), it is a couple MB.
LGTM
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1798203002/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798203002/530001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1798203002/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798203002/530001
Message was sent while issue was closed.
Description was changed from ========== Support read/write memory-mapped files. See also: chromium-dev thread "Support for Writable Memory Mapped Files", specifically the last few messages starting with "And... we're back!". BUG=546019 ========== to ========== Support read/write memory-mapped files. See also: chromium-dev thread "Support for Writable Memory Mapped Files", specifically the last few messages starting with "And... we're back!". BUG=546019 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:530001)
Message was sent while issue was closed.
Description was changed from ========== Support read/write memory-mapped files. See also: chromium-dev thread "Support for Writable Memory Mapped Files", specifically the last few messages starting with "And... we're back!". BUG=546019 ========== to ========== Support read/write memory-mapped files. See also: chromium-dev thread "Support for Writable Memory Mapped Files", specifically the last few messages starting with "And... we're back!". BUG=546019 Committed: https://crrev.com/f0dfab0bef147f55d6460b42878ef71d0578cc59 Cr-Commit-Position: refs/heads/master@{#394868} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f0dfab0bef147f55d6460b42878ef71d0578cc59 Cr-Commit-Position: refs/heads/master@{#394868} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
