New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 736452 link

Starred by 51 users

Issue metadata

Status: Fixed
Owner:
Last visit 29 days ago
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug


Show other hotlists

Hotlists containing this issue:
Hotlist-1
Hotlist-1


Sign in to add a comment

Add an option to use /tmp instead of /dev/shm

Project Member Reported by skyos...@chromium.org, Jun 23 2017

Issue description

We should make it possible to use /tmp (or another directory) instead of /dev/shm when the latter isn't available (e.g., Amazon Lambda).
 

Comment 1 by aatr...@gmail.com, Jul 12 2017

Or change to /tmp or another directory instead of /dev/shm even when /dev/shm is available, but too small (e.g. Docker).

See https://bugs.chromium.org/p/chromium/issues/detail?id=715363

Comment 2 by aatr...@gmail.com, Aug 18 2017

There appears to be a rather large group of people manually compiling Chromium just to avoid this particular issue: https://medium.com/@marco.luethy/running-headless-chrome-on-aws-lambda-fa82ad33a9eb and https://github.com/adieuadieu/serverless-chrome

This issue is also a duplicate of https://bugs.chromium.org/p/chromium/issues/detail?id=715363
skyostil@ any way we can bump the priority on this one soon? We're getting a lot of reports from developers that are trying to use Puppeteer in Docker. Many platforms would benefit: Google Cloud, Amazon Lambda, others.

I'm also starting to see it on a playground site we're working on (https://try-puppeteer.appspot.com/).

Comment 6 by pwnall@chromium.org, Sep 19 2017

Issue 764569 has a bunch of crash reports from folks trying to run Chrome in Docker. The bug has well-written repros and good debugging work, so I think it's fair to say the reporters are solid developers making reasonable use of the available documentation and resources.

Comment 7 by gane...@indix.com, Sep 19 2017

@pwnall Is 764569 private? I'm not a contributor and I can't view it.
Labels: -Pri-3 Pri-1
Agreed that this is something we want to resolve. Let me see if I can find an owner.
Owner: dvallet@chromium.org
Status: Assigned (was: Available)

Comment 10 by aatr...@gmail.com, Sep 28 2017

Awesome that this is moving forward :) I noticed this bug is assigned to headless, and just wanted to point out it occurs in a pseudo-headless environment as well, using regular Chromium + xvfb. Hopefully a fix can easily address both scenarios.
the small feature takes long time, any updates of this?

Comment 12 Deleted

Would also love to get some kind of status update, even if "it's still on the backburner". Thanks for the hard work!
We are aiming to have this done this quarter, so indeed in still on the backburner
Cc: thestig@chromium.org thomasanderson@chromium.org
+thestig does it make sense to use /tmp as a fallback for /dev/shm if it doesn't exist?

IMO, I think it does not make sense at all for chrome to use /dev/shm.  The only reason it would need to is if it shares the files with an external app, otherwise we would just be able to pass the fd from shm_open() over a pipe.  wdyt?
I strace'd Chrome and the only files created in /dev/shm were unlink()ed immediately afterwards.
Well I guess this is why we don't do shm_open()
https://cs.chromium.org/chromium/src/base/memory/shared_memory_helper.cc?rcl=53ccc526cac98cfe2e16d8c3d2c99b386a155381&l=40
But we could still do it on Linux?
Also https://bugs.chromium.org/p/chromium/issues/detail?id=522853#c25 suggested to use memfd_create on linux. Fixing this will really help for container/kubernetes use cases.
Cc: reve...@chromium.org
+reveman for his thoughts. Are there other shared memory folks who can chime in?
Using memfd_create when available should be easy and that would eliminate all these /dev/shm issues. However, memfd_create appeared in Linux 3.17 so it's not a solution for old kernels, which means we'd still have to maintain the code for using /dev/shm in chromium for now.
How can we check if memfd_create is available? I don't see many references in the codebase: https://cs.chromium.org/search/?q=+memfd_create&type=cs

We can just try to use it and fallback to /dev/shm if kernel returns ENOSYS error to indicate that syscall doesn't exits.
I'm supportive of splitting the Linux shared memory code from Mac.

For memfd_create, just try calling it and see if the kernel returns ENOSYS. Save the result in a static base::Optional<bool> so future calls can skip trying to use memfd_create if it is unavailable.

Most desktop users should have memfd_create, unless they are on Debian 8 or Ubuntu 14.04 before 14.04.5. We still need the fallback for Android/Chromecast/ChromeOS/etc where there may be devices stuck on older kernels.

The Q&A in CreateAnonymousSharedMemory() is always good for a laugh.
Thanks for the insights!
I'm working on a patch. Handling the ENOSYS error might be a bit annoying, but I guess we need to support older kernel versions.

afaik Android uses ashmem_create_region to handle shared memory
Looks like in swarm tests we always get ENOSYS for memfd_create, so the tests pass if the fallback works (see this patch's run: https://chromium-review.googlesource.com/c/chromium/src/+/781160/15)
Locally, I can test that mefd_create works for browsertests and unittest, but essentially submitting this cl would mean that the feature is untested in swarming. 

Any ideas on how to deal with this, I wonder if there are FYI bots
> essentially submitting this cl would mean that the feature is untested in swarming.

The only non-green bot I see for that patch set is android_n5x_swarming_rel.  All of the other linux and android bots are green (and most of them use swarming for tests too, even though it's not in the name), so I would say we have plenty of coverage already.  In fact, it's better that there's a config with an old kernel so that the old codepath has coverage too.
Sorry, I should've been more clear: 
That run contains a CHECK(!result) that should create a crash if memfd_create actually succeeded with no ENOSYS error
https://chromium-review.googlesource.com/c/chromium/src/+/781160/15/base/memory/shared_memory_posix.cc#143

That crashes on my local machine (with the valid kernel version) but no swarm bot actually crashes because they don't support it, and the fallback works as intended.

Cc: dpranke@chromium.org
Ah, that's not very good coverage then :)

I think you can set 'os':'Ubuntu-16.04' in swarming_dimensions to get a Xenial machine with the newer kernel.  We've been meaning to set up a Xenial config for a while but haven't gotten to it yet.  You're more than welcome to set one up.  +dpranke for visibility
Are you referring to running an isolated testing? I tried following instructions at: https://www.chromium.org/developers/testing/isolated-testing/for-swes
but the bot fails without running: https://chromium-swarm.appspot.com/task?id=3a0522d2a485ae10&refresh=10&show_raw=1

Looks like you picked up a bot with an arm cpu.  You also want 'cpu':'x86-64'.

I mean that we need to get a full FYI bot set up.
FYI sounds great. 
I'm still trying to get a run just in case before submitting, it looks like the bot is stuck on PENDING for some reason: https://chromium-swarm.appspot.com/task?id=3a147739de0d9c10&refresh=10&show_raw=1
I'll go ahead and submit the cl. 
I opened https://crbug.com/789331 to see if this can be tested in swarm
Project Member

Comment 33 by bugdroid1@chromium.org, Nov 29 2017

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

commit 0165cbc5f8bec601e0d22cea49e043821d947b55
Author: David Vallet <dvallet@chromium.org>
Date: Wed Nov 29 22:21:30 2017

Use memfd_create for anonymous shared memory files in linux, failback to temp file if not available

Test coverage:
base_unittests --gtest_filter=SharedMemoryTest.*

Bug:  736452 
Change-Id: Ic4ed3b40cc1c62ae9bfc9e2c7590e4248a8c4485
Reviewed-on: https://chromium-review.googlesource.com/781160
Commit-Queue: David Vallet <dvallet@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520269}
[modify] https://crrev.com/0165cbc5f8bec601e0d22cea49e043821d947b55/base/memory/shared_memory_helper.cc
[modify] https://crrev.com/0165cbc5f8bec601e0d22cea49e043821d947b55/base/memory/shared_memory_helper.h
[modify] https://crrev.com/0165cbc5f8bec601e0d22cea49e043821d947b55/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/0165cbc5f8bec601e0d22cea49e043821d947b55/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/0165cbc5f8bec601e0d22cea49e043821d947b55/base/memory/shared_memory_unittest.cc

c#32: Opened  bug 789768  to track adding the Xenail bot.

Now that the memfd_create patch has landed, do we still care about falling back to /tmp if /dev/shm doesn't exist and the kernel doesn't support memfd_create?
Looks like AWS lambda is using kernel 4.9 http://docs.aws.amazon.com/lambda/latest/dg/current-supported-versions.html

So in theory the fallback should not be necessary for the main use case, and probably not necessary.
 
If anyone wants to test if that's the case that'd be great.
Any idea in which stable version this will end up?
I can confirm that this change works on AWS Lambda without the need for a fallback. That's great! Thank you all for your effort.
> Any idea in which stable version this will end up?

Chrome 64.
64.0.3281.0 and newer, to be specific.
#37: Thanks for checking!

I'm inclined then to mark this as fixed, let me know if you feel otherwise
Issue 764569 has been merged into this issue.
Status: Fixed (was: Assigned)
c#40 I agree, marking as fixed.

If anyone still needs to run chrome without /dev/shm on a kernel < 3.17, please shout

Comment 43 by aatr...@gmail.com, Dec 1 2017

Just attempted to confirm that this solves the issue in Google App Engine (flexible) as well. It looks like the runtimes in GAE Docker have a kernel version of 3.16.0-4-amd64. Unfortunately, this patch doesn't actually fix the issue for that (common) environment :(. Still could really use an option to use an alternative directory than /dev/shm there.

Also, this is a duplicate/related bug in case someone wants to merge it in: https://bugs.chromium.org/p/chromium/issues/detail?id=715363
Status: Assigned (was: Fixed)
Reopening based on c#43
Cc: skyos...@chromium.org rbasuvula@chromium.org
 Issue 715363  has been merged into this issue.
> 64.0.3281.0 and newer, to be specific.

how to install it? i installed unstable version, but there is 64.0.3278.0 version
re: comment 46 - for Google Chrome, wait for an upcoming release. We have an archive of Chromium continuous builds for testing here as well: https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Linux_x64/
Project Member

Comment 48 by bugdroid1@chromium.org, Dec 5 2017

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

commit 60e1e0e9381d9965ee316752eaf3a239a764b03f
Author: David 'Digit' Turner <digit@google.com>
Date: Tue Dec 05 18:57:50 2017

Disable memfd base::SharedMemory implementation.

Since there is no way to make read-only file descriptors from Linux
memfd-based regions, the security guarantees of GetReadOnlyHandle()
cannot be maintained with this implementation.

Remove it to fall-back on traditional Posix shared regions instead.

Bug:  792117 ,  736452 
Change-Id: Ie5eb41fc3c4dd02ebdbb77be8375363ba51f1b00
Reviewed-on: https://chromium-review.googlesource.com/809014
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521768}
[modify] https://crrev.com/60e1e0e9381d9965ee316752eaf3a239a764b03f/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/60e1e0e9381d9965ee316752eaf3a239a764b03f/base/memory/shared_memory_unittest.cc

c#42

> If anyone still needs to run chrome without /dev/shm on a kernel < 3.17, please shout

*shouts* - I do. Is there a build somewhere?
An update: 
The use of memfd_create cl (https://crrev.com/c/781160)
was reverted due to security concerns, so this issue is not fixed yet. 

We might try to reland it but first SharedMemory code needs to be refractored so SEAL_WRITE can be applied after writing the data.

I'm trying to submit https://crrev.com/c/810325 which enables a flag to always use a temp directory for shmem files. You are free to patch it and try it. 


Project Member

Comment 51 by bugdroid1@chromium.org, Dec 8 2017

Labels: merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44b2c1177059576c9a5c5f9f1eefa9be29d64676

commit 44b2c1177059576c9a5c5f9f1eefa9be29d64676
Author: Robert Sesek <rsesek@chromium.org>
Date: Fri Dec 08 18:34:40 2017

Disable memfd base::SharedMemory implementation.

Since there is no way to make read-only file descriptors from Linux
memfd-based regions, the security guarantees of GetReadOnlyHandle()
cannot be maintained with this implementation.

Remove it to fall-back on traditional Posix shared regions instead.

TBR=digit@google.com

(cherry picked from commit 60e1e0e9381d9965ee316752eaf3a239a764b03f)

Bug:  792117 ,  736452 
Change-Id: Ie5eb41fc3c4dd02ebdbb77be8375363ba51f1b00
Reviewed-on: https://chromium-review.googlesource.com/809014
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#521768}
Reviewed-on: https://chromium-review.googlesource.com/817682
Cr-Commit-Position: refs/branch-heads/3282@{#100}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/44b2c1177059576c9a5c5f9f1eefa9be29d64676/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/44b2c1177059576c9a5c5f9f1eefa9be29d64676/base/memory/shared_memory_unittest.cc

Project Member

Comment 52 by bugdroid1@chromium.org, Dec 14 2017

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

commit 1824e5752148268c926f1109ed7e5ef1d937609a
Author: David Vallet <dvallet@chromium.org>
Date: Thu Dec 14 00:46:08 2017

Add switch to always use temp dir in shared memory instances on Linux builds

Modify GetShmemTempDir to check for a runtime switch to always use the temp dir
Changes tests in shared_memory_unittest.cc to be parameterized and
test also for when /dev/shm is disabled.

Bug:  736452 , 792117 
Change-Id: Id930ea525980e97c4ab70e37689301370776fc79
Reviewed-on: https://chromium-review.googlesource.com/810325
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: David Vallet <dvallet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523959}
[modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/base_switches.cc
[modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/base_switches.h
[modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/files/file_util_posix.cc
[modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/memory/shared_memory_unittest.cc

Comment 53 by aatr...@gmail.com, Dec 14 2017

Glad to see this merged!

I just tried to verify the fix in a local Docker container but am having trouble getting the --disable-dev-shm-usage switch to take effect:

Specifically, I downloaded a linux-x64 build from https://download-chromium.appspot.com/, Build Revision: 523961 (corresponds to https://chromium.googlesource.com/chromium/src/+/bb64b765223d40ce09b898dc2d2a6029a33fcc76 which should include this commit). I then loaded it into a Docker container and launched Chrome manually via "./chrome --no-sandbox --disable-dev-shm-usage" while watching the size of the 64MB /dev/shm partition. The partition's usage still increased in size when Chrome was launched, and I was able to browse around Twitter's website enough to get Chrome to run out of /dev/shm space and crash. Adding/removing the switch doesn't seem to have any effect.

Am I using the cmdline switch incorrectly, or is there some other issue?
Thanks for checking! This change should have had the same effect as the one reverted (https://crrev.com/c/781160) when running with the flag, so maybe it needs further investigation. 
It would help knowing what files are being created in /dev/shm specifically. 

What I see on my linux workstations is a "pulse-shm-*" file being created which I was assuming is created by pulseaudio, which should be disabled in containers anyway, I guess.

Comment 55 by aatr...@gmail.com, Dec 14 2017

Interestingly, when I do a "ls -al /dev/shm", I'm seeing nothing at all while Chrome is running, both with and without the flag. However, running "watch -n 0.1 df -h" shows the partition filling up as Chrome is launched and sites are loaded. I'm not familiar enough with the details of shared memory allocation in Linux to know what's causing the discrepancy, but FWIW I'm seeing the same behavior on Chrome stable as well.

Regarding the reverted change, I didn't notice it filling up /dev/shm at all when I tested it, and also confirmed that browsing behavior that reliably crashed Chrome (scrolling up and down on the Twitter signed-out homepage) did not crash Chrome. (As long as kernel >= 3.17 was used, of course.)
Thanks for the details!
I did a similar test on my workstation and I'm getting the expected behavior: /dev/shm starts filling up when not using --disable-dev-shm-usage while /tmp/ keeps constant and vice-versa when using the flag.

This is running from ToT, I'll check again once the change hits canary

Comment 57 by aatr...@gmail.com, Dec 14 2017

Sorry for the wild goose chase - you're right that the patch is working correctly. I'm not sure exactly what happened, but the build I downloaded from the link above earlier somehow didn't include your fix. When I downloaded the latest build once again just now, the switch works correctly.

Thanks for your help and for the fix!
Awesome! Thanks for double checking!
David, thanks a lot for fixing this!

Do you think the bug can be closed now?
Is this still landing in 64.0.3281.0?
Status: Fixed (was: Assigned)
It should be included in 65.0.3299.6

I'll mark this bug as fixed and create https://crbug.com/798221 to track using memfd_create in the future
Just to check will this fix also apply to the GAE environment?
Yep, you should be able to add the flag when running in GAE.  Are you
seeing issues?
@dvallet, we're trying to figure out if we should add this flag by default for Puppeteer users on Linux. It would reduce the number of Docker issues we get :)

https://github.com/GoogleChrome/puppeteer/issues/1834

Can you help me understand:

- Are there implications/drawbacks when using the flag?
- Why this isn't Chrome's new default behavior?
- Does it work with headful Chrome?
I think it would depend on how are /dev/shm and /tmp mounted. 
If they are both mounted as tmpfs I'm assuming there won't be any difference. 
if for some reason /tmp is not mapped as tmpfs (and I think is mapped as tmpfs by default by systemd), chrome shared memory management always maps files into memory when creating an anonymous shared files, so even in that case shouldn't be much difference. I guess you could force telemetry tests with the flag enabled and see how it goes.

As for why not use by default, it was a pushed back by the shared memory team, I guess it makes sense it should be useing /dev/shm for shared memory by default.

Ultimately all this should be moving to use memfd_create, but I don't think that's going to happen any time soon, since it will require refactoring Chrome memory management significantly. 


Project Member

Comment 66 by bugdroid1@chromium.org, Feb 22 2018

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

commit 45a7b305282b175e11c871c1a8ac3b8336c66259
Author: Wez <wez@chromium.org>
Date: Thu Feb 22 22:42:39 2018

Fix CreateAnonymousSharedMemory() not to leak FILE when returning fd.

CreateAnonymousSharedMemory() was modified to return the writable memory
handle as a file-descriptor rather than as a FILE. Since POSIX does not
provide a standard way to teardown a FILE without also close()ing the
underlying file-descriptor, this was achieved by leaking the FILE.

We now provide CreateAndOpenFdForTemporaryFileInDir(), to avoid the need
to wrap the temporary-file descriptor into a FILE at all.

Bug: 814444,  736452 
Change-Id: Idd911f50f0e506de3eac91c809efb6c6d59fec10
Reviewed-on: https://chromium-review.googlesource.com/930336
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538599}
[modify] https://crrev.com/45a7b305282b175e11c871c1a8ac3b8336c66259/base/files/file_util.h
[modify] https://crrev.com/45a7b305282b175e11c871c1a8ac3b8336c66259/base/files/file_util_posix.cc
[modify] https://crrev.com/45a7b305282b175e11c871c1a8ac3b8336c66259/base/memory/shared_memory_helper.cc

Project Member

Comment 67 by bugdroid1@chromium.org, Feb 28 2018

Labels: merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fca5d89ce30c4af1f53750de111e774b031f70f5

commit fca5d89ce30c4af1f53750de111e774b031f70f5
Author: Wez <wez@chromium.org>
Date: Wed Feb 28 00:35:23 2018

Fix CreateAnonymousSharedMemory() not to leak FILE when returning fd.

CreateAnonymousSharedMemory() was modified to return the writable memory
handle as a file-descriptor rather than as a FILE. Since POSIX does not
provide a standard way to teardown a FILE without also close()ing the
underlying file-descriptor, this was achieved by leaking the FILE.

We now provide CreateAndOpenFdForTemporaryFileInDir(), to avoid the need
to wrap the temporary-file descriptor into a FILE at all.

Bug: 814444,  736452 
Change-Id: Idd911f50f0e506de3eac91c809efb6c6d59fec10
Reviewed-on: https://chromium-review.googlesource.com/930336
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#538599}(cherry picked from commit 45a7b305282b175e11c871c1a8ac3b8336c66259)
Reviewed-on: https://chromium-review.googlesource.com/940163
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#615}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/fca5d89ce30c4af1f53750de111e774b031f70f5/base/files/file_util.h
[modify] https://crrev.com/fca5d89ce30c4af1f53750de111e774b031f70f5/base/files/file_util_posix.cc
[modify] https://crrev.com/fca5d89ce30c4af1f53750de111e774b031f70f5/base/memory/shared_memory_helper.cc

Sign in to add a comment