Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1741403002:
base: Avoid calling VirtualAlloc with 0 length. (Closed)
Descriptionbase: Avoid calling VirtualAlloc with 0 length.
BUG=577786, 590792
TEST=base_unittests --gtest_filter=DiscardableSharedMemoryTest.ZeroSize
Committed: https://crrev.com/4ffe952ecf9319a9f6eed847e7a6d6d20b893e4c
Cr-Commit-Position: refs/heads/master@{#378595}
Patch Set 1 #
Total comments: 5
Patch Set 2 : return PURGED in Lock #Patch Set 3 : keep most of unit test #
Total comments: 2
Messages
Total messages: 23 (8 generated)
The CQ bit was checked by reveman@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/1741403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741403002/1
reveman@chromium.org changed reviewers: + pennymac@chromium.org
pennymac@, wdyt? I'm hoping this will fix: https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%2010/builds...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So I suppose I think we should be catching these "bad" arguments, in case they
ever happen in the wild. In both Lock and Unlock.
Nobody should be calling lock/unlock with an offset at the very end of the
mapping, and length of 0 ("everything onward"). Unless you can think of a
legitimate reason for doing this?
Then the test case could be adjusted as well.
Let me know what you think David.
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha...
File base/memory/discardable_shared_memory.cc (right):
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha...
base/memory/discardable_shared_memory.cc:205: length =
AlignToPageSize(mapped_size_) - offset;
The only time length can be 0 is if the caller passes in offset == the very end
of the mapping, and length == 0 (i.e. "everything onward").
I think that would be a "bad" function call, and we should perhaps be returning
FAILED (and DCHECKing to try to catch any culprits in the wild?).
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha...
base/memory/discardable_shared_memory.cc:256: length =
AlignToPageSize(mapped_size_) - offset;
Here too.
On 2016/02/28 18:44:01, penny wrote:
> So I suppose I think we should be catching these "bad" arguments, in case they
> ever happen in the wild. In both Lock and Unlock.
>
> Nobody should be calling lock/unlock with an offset at the very end of the
> mapping, and length of 0 ("everything onward"). Unless you can think of a
> legitimate reason for doing this?
>
> Then the test case could be adjusted as well.
>
> Let me know what you think David.
>
>
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha...
> File base/memory/discardable_shared_memory.cc (right):
>
>
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha...
> base/memory/discardable_shared_memory.cc:205: length =
> AlignToPageSize(mapped_size_) - offset;
> The only time length can be 0 is if the caller passes in offset == the very
end
> of the mapping, and length == 0 (i.e. "everything onward").
>
> I think that would be a "bad" function call, and we should perhaps be
returning
> FAILED (and DCHECKing to try to catch any culprits in the wild?).
>
>
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha...
> base/memory/discardable_shared_memory.cc:256: length =
> AlignToPageSize(mapped_size_) - offset;
> Here too.
And just to clarify, the test in question is "TEST(DiscardableSharedMemoryTest,
ZeroSize)".
https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:205: length = AlignToPageSize(mapped_size_) - offset; On 2016/02/28 at 18:44:00, penny wrote: > The only time length can be 0 is if the caller passes in offset == the very end of the mapping, and length == 0 (i.e. "everything onward"). > > I think that would be a "bad" function call, and we should perhaps be returning FAILED (and DCHECKing to try to catch any culprits in the wild?). We've had DCHECKs for this at a higher level but they went unnoticed as DCHECKs are not used in the wild. These DCHECKs are for that reason temporarily CHECKs as I'd like to fix any code that allocates 0 bytes of discardable memory. However, even if we fix the issues we have today I'm worried this might pop up again in the future as it's not too crazy to expect 0 byte allocations to succeed as that's valid for malloc. So I'd like to have DCHECKs for 0 size allocations at a higher level but not have the discardable code crash in the wild if this happens. Does that make sense?
Sure, I'm on board with tracking down delinquents at a higher level. Just see my comment below about returning PURGED if length is zero. Slightly safer in a bad situation. Otherwise, LGTM. https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:235: if (length && if (!length) return PURGED; Unlock is fine as is, but perhaps in Lock we should return PURGED in the rare case we ever get a length of zero at this point. This will just prevent the misbehaving caller from thinking the memory contents of this mapping have been preserved (implied by SUCCESS), when there's a chance the contents have in fact been lost and need to be recreated.
reveman@chromium.org changed reviewers: + thakis@chromium.org
+thakis for base/ https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1741403002/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:235: if (length && On 2016/02/28 at 21:49:10, penny wrote: > if (!length) > return PURGED; > > Unlock is fine as is, but perhaps in Lock we should return PURGED in the rare case we ever get a length of zero at this point. > > This will just prevent the misbehaving caller from thinking the memory contents of this mapping have been preserved (implied by SUCCESS), when there's a chance the contents have in fact been lost and need to be recreated. Done. Note: memory segments are initially locked so you can still have a locked 0-byte segment.
Description was changed from ========== content: Avoid calling VirtualAlloc with 0 length. BUG=577786 TEST=base_unittests --gtest_filter=DiscardableSharedMemoryTest.ZeroSize ========== to ========== base: Avoid calling VirtualAlloc with 0 length. BUG=577786,590792 TEST=base_unittests --gtest_filter=DiscardableSharedMemoryTest.ZeroSize ==========
https://codereview.chromium.org/1741403002/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1741403002/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:205: length = AlignToPageSize(mapped_size_) - offset; this writes to length_ if it's 0 and nothing after this writes to it. Does this mean mapped_size_ - offset is 0 here? should your change be here, since this already tries to handle that case? What happens if callers pass in an offset larget than mapped_size_, won't this underflow?
https://codereview.chromium.org/1741403002/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1741403002/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:205: length = AlignToPageSize(mapped_size_) - offset; On 2016/02/29 at 20:10:13, Nico wrote: > this writes to length_ if it's 0 and nothing after this writes to it. Does this mean mapped_size_ - offset is 0 here? correct. > should your change be here, since this already tries to handle that case? > > What happens if callers pass in an offset larget than mapped_size_, won't this underflow? The DCHECK below will catch that and that's also why I prefer having the "if (!length) return PURGED" below. I'd rather not duplicate that DCHECK logic here only for the case where length==0. Does that make sense?
will this be committed soon? This is the final breaking CL on the win10 bot and I would like it to be green asap. I am happy to just revert 2ac5d1f168f935be73cb6425a97fe5bae8fe081c if that's easier.
lgtm
The CQ bit was checked by reveman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pennymac@chromium.org Link to the patchset: https://codereview.chromium.org/1741403002/#ps40001 (title: "keep most of unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1741403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741403002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== base: Avoid calling VirtualAlloc with 0 length. BUG=577786,590792 TEST=base_unittests --gtest_filter=DiscardableSharedMemoryTest.ZeroSize ========== to ========== base: Avoid calling VirtualAlloc with 0 length. BUG=577786,590792 TEST=base_unittests --gtest_filter=DiscardableSharedMemoryTest.ZeroSize Committed: https://crrev.com/4ffe952ecf9319a9f6eed847e7a6d6d20b893e4c Cr-Commit-Position: refs/heads/master@{#378595} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4ffe952ecf9319a9f6eed847e7a6d6d20b893e4c Cr-Commit-Position: refs/heads/master@{#378595} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
