base::SharedMemory and IPC

[Daniel Cheng]

This might be dumb, but I'm a bit confused about the Right Way (tm) to use base::SharedMemory across IPC.

I've been trying to refactor some clipboard IPC code to be more readable (https://codereview.chromium.org/574273002), and it occurred to me that I have no idea what the right way to do things here is. Here's some of the things I'm confused about. I'm hoping some experts can shed some light for me...

- base::SharedMemory has both GiveToProcess() and ShareToProcess(). However, the clipboard code currently just passes base::SharedMemory::handle() to the IPC message. Am I supposed to be calling one of the GiveTo/ShareTo methods here?

- The security advice for IPCs (http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc) explicitly says that you shouldn't pass sizes to base::SharedMemory::Map(). What are my alternatives then?

- In fact, base::SharedMemory::Map() doesn't seem to (or can't?) validate that the size parameter passed in isn't > the actual size of the shared memory backing it... one of the new unit tests I added crashes with BUG_ADRERR when it tries to read pass the end of the shmem segment. 

- Finally, sending a SharedMemory handle across IPC always requires a sync IPC, right? I can't imagine it working any other way.

Daniel

[John Bauman]

On Mon, Sep 29, 2014 at 2:09 PM, Daniel Cheng <dch...@chromium.org> wrote:

This might be dumb, but I'm a bit confused about the Right Way (tm) to use base::SharedMemory across IPC.

I've been trying to refactor some clipboard IPC code to be more readable (https://codereview.chromium.org/574273002), and it occurred to me that I have no idea what the right way to do things here is. Here's some of the things I'm confused about. I'm hoping some experts can shed some light for me...

- base::SharedMemory has both GiveToProcess() and ShareToProcess(). However, the clipboard code currently just passes base::SharedMemory::handle() to the IPC message. Am I supposed to be calling one of the GiveTo/ShareTo methods here?

- The security advice for IPCs (http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc) explicitly says that you shouldn't pass sizes to base::SharedMemory::Map(). What are my alternatives then?

On Windows  you can to Map(0), which will map the entire file. I don't believe there's an alternative to passing the size on posix.

- In fact, base::SharedMemory::Map() doesn't seem to (or can't?) validate that the size parameter passed in isn't > the actual size of the shared memory backing it... one of the new unit tests I added crashes with BUG_ADRERR when it tries to read pass the end of the shmem segment. 

In theory the other process could even create a file of the right size, then ftruncate it shorter later. At least if you pass in the size you can guarantee that any accesses to that region than that will at worst cause a bus error and won't access other memory.

- Finally, sending a SharedMemory handle across IPC always requires a sync IPC, right? I can't imagine it working any other way.

This is where ShareToProcess and GiveToProcess can be useful. If you create a new handle using one of those and pass it to the other process, then it will live as long as the other process does, and the other process can open it whenever it wants. No sync IPC necessary.

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Daniel Cheng]

​Hm. So this inspires two followup questions from looking at how things are used in Chrome:

1) Is doesn't look like it's possible to get a ProcessHandle in content/renderer. Is this intentional? I assume it is to prevent a renderer from attaching large shared memory blobs to random processes.

​2) ​I notice there are several calls to GiveToProcess(base::GetCurrentProcessHandle(), ...) in non-test code. Why would you want to do that?

Daniel

[John Bauman]

On Mon, Sep 29, 2014 at 5:16 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Sep 29, 2014 at 3:52 PM, John Bauman <jba...@chromium.org> wrote:

On Mon, Sep 29, 2014 at 2:09 PM, Daniel Cheng <dch...@chromium.org> wrote:

This might be dumb, but I'm a bit confused about the Right Way (tm) to use base::SharedMemory across IPC.

I've been trying to refactor some clipboard IPC code to be more readable (https://codereview.chromium.org/574273002), and it occurred to me that I have no idea what the right way to do things here is. Here's some of the things I'm confused about. I'm hoping some experts can shed some light for me...

- base::SharedMemory has both GiveToProcess() and ShareToProcess(). However, the clipboard code currently just passes base::SharedMemory::handle() to the IPC message. Am I supposed to be calling one of the GiveTo/ShareTo methods here?

- The security advice for IPCs (http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc) explicitly says that you shouldn't pass sizes to base::SharedMemory::Map(). What are my alternatives then?

On Windows  you can to Map(0), which will map the entire file. I don't believe there's an alternative to passing the size on posix.

- In fact, base::SharedMemory::Map() doesn't seem to (or can't?) validate that the size parameter passed in isn't > the actual size of the shared memory backing it... one of the new unit tests I added crashes with BUG_ADRERR when it tries to read pass the end of the shmem segment. 

In theory the other process could even create a file of the right size, then ftruncate it shorter later. At least if you pass in the size you can guarantee that any accesses to that region than that will at worst cause a bus error and won't access other memory.

- Finally, sending a SharedMemory handle across IPC always requires a sync IPC, right? I can't imagine it working any other way.

This is where ShareToProcess and GiveToProcess can be useful. If you create a new handle using one of those and pass it to the other process, then it will live as long as the other process does, and the other process can open it whenever it wants. No sync IPC necessary.

​Hm. So this inspires two followup questions from looking at how things are used in Chrome:

1) Is doesn't look like it's possible to get a ProcessHandle in content/renderer. Is this intentional? I assume it is to prevent a renderer from attaching large shared memory blobs to random processes.

 

I believe so. The browser can duplicate the handle from the renderer if it needs to open it on Windows, and on POSIX it doesn't really matter.

 

​2) ​I notice there are several calls to GiveToProcess(base::GetCurrentProcessHandle(), ...) in non-test code. Why would you want to do that?

This is a way to grab the handle out of the SharedMemory, so the SharedMemory won't close it when destructed. The handle can then be passed to the browser in an IPC, and the browser can open it (using the 3-argument constructor on windows).

Unfortunately POSIX and windows shared memory semantics are very different, so they have slightly different rules.