New issue
Advanced search Search tips

Issue 866975 link

Starred by 1 user
Status: Started
Owner:
dcheng@chromium.org
Cc:
rsesek@chromium.org
alexilin@chromium.org
tsepez@chromium.org
palmer@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Feature



Sign in to add a comment

Make SharedMemoryMapping more type safer (and DiscardableMemory)

Project Member Reported by dcheng@chromium.org, Jul 24 
Right now, we rely on people to remember to check:
- if the mapping is valid
- if the mapping is large enough
- then static_cast from void* to the type of choice

https://chromium-review.googlesource.com/c/chromium/src/+/1144312 encapsulates these checks inside the GetMemoryAs<T>/GetMemoryAsSpan<T> helpers, so we should switch uses of SharedMemoryMapping::memory() over to them.

One interesting question is what to do for the current test usage, which just tries to compare addresses...

Also, alexilin points out that there is also DiscardableMemory::data_as, which we should replace with a similar mechanism.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcdae7e0d22984bca7101baaf528ff7ca7e1a175

commit fcdae7e0d22984bca7101baaf528ff7ca7e1a175
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Jul 24 19:11:22 2018

Add type-safer helpers for accessing SharedMemoryMappings.

- GetMemoryAs<T> should be used when the mapping contains one T.
- GetMemoryAsSpan<T> should be used when the mapping contains more than
  one T. The element count can be explicitly specified or autodeduced
  from the size of the shared memory mapping.

Bug: 795291, 866975
Change-Id: Icea4e276a26a1c47688c0a5206b97cd42ff92043
Reviewed-on: https://chromium-review.googlesource.com/1144312
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Alexandr Ilin <alexilin@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577630}
[modify] https://crrev.com/fcdae7e0d22984bca7101baaf528ff7ca7e1a175/base/BUILD.gn
[modify] https://crrev.com/fcdae7e0d22984bca7101baaf528ff7ca7e1a175/base/memory/shared_memory_mapping.h
[add] https://crrev.com/fcdae7e0d22984bca7101baaf528ff7ca7e1a175/base/memory/shared_memory_mapping_unittest.cc

Comment 2 by palmer@chromium.org, Jul 30

Components: Security
Labels: -Type-Bug OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Type-Feature
Owner: dcheng@chromium.org
Status: Started (was: Available)
dcheng: Is there more to be done here, or do we call this Fixed?

Comment 3 by dcheng@chromium.org, Jul 30 

We still need to fix DiscardableMemory in a comparable way and remove all uses of the old idioms. I was planning on tracking that here as well.

Comment 4 by alexilin@chromium.org, Nov 19

Cc: alexilin@chromium.org
►

Sign in to add a comment