I don't understand. As long as the region is writable, the mapping is writable. The permissions on the mapping are controlled by the region, and one can't downgrade the mapping without downgrading the region (on android at least, which is what limits the cross-platform API).

Oh right, I forgot about how ashmem works.

That's unfortunate then: the two alternatives that allow the use of WritableSharedMemoryRegion are rather ugly and not something I'd generally recommend. I guess we can see if the code in question can be modified to pass the region from the renderer to the browser...

Wait a second, I believe that the permissions on the mapping are controlled by the flags passed into mmap() function. 

ashmem's specialty is that any process holding a handle to the region can map it as writable until ashmem_set_prot_region() is lowered. It doesn't mean that the region cannot be mapped as read-only.

At least I'd like to verify that this is impossible.

Can a process flip the protection bits back on a memory mapping, though? I'll fiddle around a bit to find out.

You're right, even on Linux one can call mprotect(addr, len, PROT_READ | PROT_WRITE) on the mapped page and it succeeds if the region handle had had writable permissions.

I'm wondering whether there is a way to prevent this.

I think the basic problem is that on android we just need the extra IPC. We can't safely map a readonly region until we get a an ACK from the process that downgrades the region to readonly. So we're always going to need an IPC to transport the ACK.

Does the browser strictly speaking need to keep a read-only mapping? Could instead the browser create a Writable region, map it (as writable), and then transfer the writable region to the renderer? It wouldn't fully reflect the producer/consumer relationship, but it could avoid the use of Unsafe.

Re: #5: I think you'd need to reopen the descriptor from /dev/fd or /proc/self/fd as RDONLY. fcntl won't allow you to change the access flags on a descriptor, and mprotect doesn't differentiate between effective and max protection like Mach VM does.

#7 works, in that I've confirmed on android that a writable region accessed through a readonly fd can't be mapped writable. So we could have a DuplicateAsReadOnly method on a WritableSharedMemoryRegion that reopens the fd as readonly. 

However, if that readonly fd was then shipped to a rogue process wouldn't it be able to be mapped writable? (on android at least) That makes me think that it's safest to have the browser just keep a writable mapping if it needs to have a mapping, as suggested in #7.

> However, if that readonly fd was then shipped to a rogue process wouldn't it be able to be mapped writable? (on android at least) 

Yeah, that's right I think because of the "third layer" of the actual ashmem object. 

Also, reopening the descriptor wouldn't work inside a renderer process on Linux/CrOS because they do not have access to the filesystem at all.

I'm okay with doing what's suggested in #7. It's a little unfortunate that we cannot encode the intent in the type system, but this is really a hard thing to model cross-platform. We should update the docs if we want to prosthelytize that pattern, too.

An alternate proposal: maybe we should just prefer sending ReadOnlySharedMemoryRegion where possible and minimize the transport of mutable shmem regions over IPC?

This means that the original patch should be written so that the renderer uses the brokered Mojo shmem creation path and sends a readonly region to the browser, rather than having the browser initiate it and send a writable region to the renderer. I don't know how difficult that would be here (yuzus@, do you have thoughts on this?)

Finally, I know there was some concern that the sync brokering IPCs would be problematic: has that proven to be the case in practice?

> This means that the original patch should be written so that the renderer uses the brokered Mojo shmem creation path and sends a readonly region to the browser, rather than having the browser initiate it and send a writable region to the renderer. I don't know how difficult that would be here (yuzus@, do you have thoughts on this?)

AFAIU that's not possible, the reason why a broker is used in the first place is because the renderer doesn't have permissions to create shared memory and so must ask a privileged process (the browser) to do it for it. 

>Finally, I know there was some concern that the sync brokering IPCs would be problematic: has that proven to be the case in practice?

I don't think so, but Ken will know for sure.

I don't think there are any serious concerns over the sync brokering. Dead-locks aren't possible and IPC latency is unavoidable. The only potential concern I can think of then would be thread jank, but in my opinion that's a concern for the application to work out. i.e. if you identify some shared buffer allocation as a source of thread jank, you can figure out how to move it to a background thread.

In practice renderers have been doing main-thread sync IPCs since forever, including for shm allocations, and they are not really an issue**.

** A DoSed browser will cause serious jank for sync IPCs, but browser DoS has many worse side effects.
** We also see jank with some specific problematic sync IPCs, like cookie access when the disk is tied up.

> AFAIU that's not possible, the reason why a broker is used in the first place is because the renderer doesn't have permissions to create shared memory and so must ask a privileged process (the browser) to do it for it. 

To be clear, I mean that the renderer 'creates' it by using the brokered Mojo APIs: this reverses control flow from the current model (where the browser pushes the region to the renderer) to the renderer pushing the region to the browser. But you are right, in that the browser process is ultimately the one creating the region.