Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770753006:
[base/files] Respect MAC_CHROMIUM_TMPDIR instead of TMPDIR on macOS. (Closed)
Description[base/files] Respect MAC_CHROMIUM_TMPDIR instead of TMPDIR on macOS.
Some folks are currently setting $TMPDIR to very long values on macOS,
which causes socket name length issues when using $TMPDIR.
R=mattm@chromium.org
BUG=698759, 696803
Review-Url: https://codereview.chromium.org/2770753006
Cr-Commit-Position: refs/heads/master@{#461515}
Committed: https://chromium.googlesource.com/chromium/src/+/16abc4a0f8b419fe03dcfba8799cae52f28993b1
Patch Set 1 #
Total comments: 3
Patch Set 2 : feedback #
Total comments: 6
Patch Set 3 : import #Patch Set 4 : import and uint #Patch Set 5 : fix DeveloperPrivateApiUnitTest.LoadUnpackedRetryId #
Total comments: 1
Patch Set 6 : hoist! #
Total comments: 1
Patch Set 7 : comment #Messages
Total messages: 73 (38 generated)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Description was changed from ========== [base/files] Respect MAC_CHROMIUM_TMPDIR instead of TMPDIR on macOS. Some folks are currently setting $TMPDIR to very long values on macOS, which causes socket name length issues when using $TMPDIR. R=mattm@chromium.org BUG=698759, 696803 ========== to ========== [base/files] Respect MAC_CHROMIUM_TMPDIR instead of TMPDIR on macOS. Some folks are currently setting $TMPDIR to very long values on macOS, which causes socket name length issues when using $TMPDIR. R=mattm@chromium.org BUG=698759, 696803 ==========
iannucci@google.com changed reviewers: + thakis@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+thakis for FYI
https://codereview.chromium.org/2770753006/diff/1/base/files/file_util_mac.mm File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2770753006/diff/1/base/files/file_util_mac.mm... base/files/file_util_mac.mm:29: const char* env_tmpdir = getenv("MAC_CHROMIUM_TMPDIR"); I don't have strong feelings about this name.. if someone else does, I'm happy to change it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Monday morning ping/ptal :)
On 2017/03/27 17:02:57, iannucci wrote: > Monday morning ping/ptal :) lgtm. I don't have a strong feeling about the name either.
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/03/27 19:55:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) waaat
On 2017/03/27 20:28:07, iannucci wrote: > On 2017/03/27 19:55:22, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > waaat Missing LGTM from an OWNER for these files: base/files/file_util_mac.mm
iannucci@chromium.org changed reviewers: + mark@chromium.org
+mark for OWNERS
but LGTM https://codereview.chromium.org/2770753006/diff/1/base/files/file_util_mac.mm File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2770753006/diff/1/base/files/file_util_mac.mm... base/files/file_util_mac.mm:28: // $MAC_CHROMIUM_TMPDIR. Explain why not TMPDIR here, since it’s a natural question to ask, and our kids shouldn’t have to spelunk through file history to get the answer. https://codereview.chromium.org/2770753006/diff/1/base/files/file_util_mac.mm... base/files/file_util_mac.mm:29: const char* env_tmpdir = getenv("MAC_CHROMIUM_TMPDIR"); Maybe [D]CHECK that the length is within a reasonable range here, to avoid hard-to-diagnose problems like bug 698759.
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... base/files/file_util_mac.mm:33: DCHECK_LT(strlen(env_tmpdir), 50) NSTemporaryDirectory() length is 49.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
LGTM otherwise. Thanks for the updates. https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... base/files/file_util_mac.mm:10: #include "base/files/file_util.h" This was correct where it was before. https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... base/files/file_util_mac.mm:33: DCHECK_LT(strlen(env_tmpdir), 50) #inclue <string.h> for this.
https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... base/files/file_util_mac.mm:10: #include "base/files/file_util.h" On 2017/03/28 00:20:23, Mark Mentovai wrote: > This was correct where it was before. Presumit was hollering at me about this (my original patchset had it above the system imports). https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... base/files/file_util_mac.mm:33: DCHECK_LT(strlen(env_tmpdir), 50) On 2017/03/28 00:20:23, Mark Mentovai wrote: > #inclue <string.h> for this. er... whoops.
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... base/files/file_util_mac.mm:10: #include "base/files/file_util.h" On 2017/03/28 00:24:04, iannucci wrote: > On 2017/03/28 00:20:23, Mark Mentovai wrote: > > This was correct where it was before. > > Presumit was hollering at me about this (my original patchset had it above the > system imports). Presubmit’s wrong, NOPRESUBMIT it if you have to, and we can figure out how to fix it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/28 00:27:14, Mark Mentovai wrote: > https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... > File base/files/file_util_mac.mm (right): > > https://codereview.chromium.org/2770753006/diff/20001/base/files/file_util_ma... > base/files/file_util_mac.mm:10: #include "base/files/file_util.h" > On 2017/03/28 00:24:04, iannucci wrote: > > On 2017/03/28 00:20:23, Mark Mentovai wrote: > > > This was correct where it was before. > > > > Presumit was hollering at me about this (my original patchset had it above the > > system imports). > > Presubmit’s wrong, NOPRESUBMIT it if you have to, and we can figure out how to > fix it. Discussed offline; I did a silly thing. PRESUBMIT is fine.
The CQ bit was unchecked by iannucci@chromium.org
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/2770753006/#ps60001 (title: "import and uint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
@mattm; any idea what's happening here: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2... ? It sortof looks like something is expanding (i.e. `realpath` or `readlink`) the path value from NSTemporaryDirectory and then asserting on that; I suspect that this assertion snuck in during the time that we were respecting TMPDIR and now it's breaking (because we don't yet set MAC_CHROMIUM_TMPDIR on bots). I could set MAC_CHROMIUM_TMPDIR on bots (to the same value that TMPDIR is today), which would 'fix' it :), but I suspect that this test may also fail locally?
iannucci@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin.cronin for developer_private_api_unittest.cc OWNERS Basically on OS X the temp directory includes a symlink (i.e. /var/folders/9s/n0cjtdts60l2zdskcypggysc0056bp/T/ where /var is a symlink `var/private`). When the extension is loaded the path is resolved with base::MakeAbsoluteFilePath (here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/unpacked_insta...), the assertion in the test here (https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/developer_...) fails (because `path` contains a symlink, but extension->path() does not). Also, I have no idea what I'm doing, so if this is the wrong way to fix this, please advise :D.
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I wish people would stop with all of these absolute/resolved paths all over the place…
On 2017/03/29 04:25:24, Mark Mentovai wrote: > I wish people would stop with all of these absolute/resolved paths all over the > place… Do unresolved paths present a potential race (i.e TOCTOU) which people are attempting to avoid? Is there a better systematic solution?
On 2017/03/29 04:29:02, iannucci wrote: > On 2017/03/29 04:25:24, Mark Mentovai wrote: > > I wish people would stop with all of these absolute/resolved paths all over > the > > place… > > Do unresolved paths present a potential race (i.e TOCTOU) which people are > attempting to avoid? Is there a better systematic solution? I can’t speak to how it’s being used here, but in most of the cases I’ve seen, resolved paths were just being used as crutches to paper over something “hard.” I’m sensitive to races, but if there’s a concern that any part of a path hierarchy might be open to unexpected changes, absolute or resolved aren’t going to help.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2770753006/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2770753006/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:607: base::FilePath path = base::MakeAbsoluteFilePath(dir.UnpackedPath()); Hmm... this doesn't seem like quite the right place to do this. A few different options: - At minimum, we should probably hoist this up to TestExtensionDir::UnpackedPath (and probably apply a similar change to the value returned by Pack()). I *think* that will be a fairly easy change that doesn't break anything. - That said, TestExtensionDir basically just relies on ScopedTempDir - would it make sense to have ScopedTempDir return the absolute path to prevent this kind of misuse? I don't know if anything is currently relying on a non-absolute path being returned by ScopedTempDir... - How crazy would it be for FilePath::operator== to return true if the absolute paths are the same? I'd probably be fine with any of these options, but if we go for the less general-case one (modifying TestExtensionDir), it'd be good to know the reasoning for why the others would be incorrect.
On 2017/03/29 15:15:43, Devlin wrote: > https://codereview.chromium.org/2770753006/diff/80001/chrome/browser/extensio... > File > chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc > (right): > > https://codereview.chromium.org/2770753006/diff/80001/chrome/browser/extensio... > chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:607: > base::FilePath path = base::MakeAbsoluteFilePath(dir.UnpackedPath()); > Hmm... this doesn't seem like quite the right place to do this. A few different > options: > - At minimum, we should probably hoist this up to TestExtensionDir::UnpackedPath > (and probably apply a similar change to the value returned by Pack()). I > *think* that will be a fairly easy change that doesn't break anything. > - That said, TestExtensionDir basically just relies on ScopedTempDir - would it > make sense to have ScopedTempDir return the absolute path to prevent this kind > of misuse? I don't know if anything is currently relying on a non-absolute path > being returned by ScopedTempDir... > - How crazy would it be for FilePath::operator== to return true if the absolute > paths are the same? > > I'd probably be fine with any of these options, but if we go for the less > general-case one (modifying TestExtensionDir), it'd be good to know the > reasoning for why the others would be incorrect. So the behavior of TMPDIR on OS X only changed ~2 weeks ago; it used to return the symlinky version always (i.e. /var/blah/blah/...). Inside that 2 week stint it's been a short non-symlinky path. I don't really have a lot of basis for input here, but: * I'd be fine hoisting it up to UnpackedPath, if that's the right thing to do. * Changing ScopedTempDir seems possibly wrong, but I don't know enough to say why. It would probably be fine. * Changing == seems very wrong, because it would take a currently-cheap memcmp-ish operator and throw some filesystem stats in the middle. I have no idea what the performance implications here would be, but it seems like it could be bad (and especially bad on windows). Another possibility would be to guard the absolute-pathify-ing logic with an IsAbsolute, which only checks to see if the path is absolute, unlike MakeAbsoluteFilePath which also applies 'realpath' (which is confusing to me, IMO MakeAbsoluteFilePath should check to see if IsAbsolute is true and then return the original path).
To be clear: this CL is a release blocker (for 59), so I'd like to get this fixed sooner rather than later :)
On 2017/03/29 16:52:39, iannucci wrote: > On 2017/03/29 15:15:43, Devlin wrote: > > > https://codereview.chromium.org/2770753006/diff/80001/chrome/browser/extensio... > > File > > > chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2770753006/diff/80001/chrome/browser/extensio... > > > chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:607: > > base::FilePath path = base::MakeAbsoluteFilePath(dir.UnpackedPath()); > > Hmm... this doesn't seem like quite the right place to do this. A few > different > > options: > > - At minimum, we should probably hoist this up to > TestExtensionDir::UnpackedPath > > (and probably apply a similar change to the value returned by Pack()). I > > *think* that will be a fairly easy change that doesn't break anything. > > - That said, TestExtensionDir basically just relies on ScopedTempDir - would > it > > make sense to have ScopedTempDir return the absolute path to prevent this kind > > of misuse? I don't know if anything is currently relying on a non-absolute > path > > being returned by ScopedTempDir... > > - How crazy would it be for FilePath::operator== to return true if the > absolute > > paths are the same? > > > > I'd probably be fine with any of these options, but if we go for the less > > general-case one (modifying TestExtensionDir), it'd be good to know the > > reasoning for why the others would be incorrect. > > So the behavior of TMPDIR on OS X only changed ~2 weeks ago; it used to return > the symlinky version always (i.e. /var/blah/blah/...). Inside that 2 week stint > it's been > a short non-symlinky path. > > I don't really have a lot of basis for input here, but: > * I'd be fine hoisting it up to UnpackedPath, if that's the right thing to do. > * Changing ScopedTempDir seems possibly wrong, but I don't know enough to say > why. It would probably be fine. > * Changing == seems very wrong, because it would take a currently-cheap > memcmp-ish operator and throw some filesystem stats in the middle. I have no > idea what the performance implications here would be, but it seems like it could > be bad (and especially bad on windows). > > Another possibility would be to guard the absolute-pathify-ing logic with an > IsAbsolute, which only checks to see if the path is absolute, unlike > MakeAbsoluteFilePath which also applies 'realpath' (which is confusing to me, > IMO MakeAbsoluteFilePath should check to see if IsAbsolute is true and then > return the original path). Let's go with the easiest of all the options - hoist this up to TestExtensionDir::UnpackedPath(). :)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Finally got back to this, PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % nit, assuming the bots are happy. https://codereview.chromium.org/2770753006/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/test_extension_dir.cc (right): https://codereview.chromium.org/2770753006/diff/100001/chrome/browser/extensi... chrome/browser/extensions/test_extension_dir.cc:70: return base::MakeAbsoluteFilePath(dir_.GetPath()); Can we add a comment explaining why this is important?
On 2017/04/03 17:25:22, Devlin wrote: > lgtm % nit, assuming the bots are happy. > > https://codereview.chromium.org/2770753006/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/test_extension_dir.cc (right): > > https://codereview.chromium.org/2770753006/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/test_extension_dir.cc:70: return > base::MakeAbsoluteFilePath(dir_.GetPath()); > Can we add a comment explaining why this is important? Added a comment, but please take a look to review it for accuracy+helpfulness... I'm new in this code :)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bots are happy, I'll commit this as-is to prevent any other regressions and free up 59. If the comment needs improvement I'm more than happy to improve it in a followup :)
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, mattm@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2770753006/#ps120001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491249005554580,
"parent_rev": "3841ef227593b7d49d2b7010a8f405c842c7bb3e", "commit_rev":
"16abc4a0f8b419fe03dcfba8799cae52f28993b1"}
Message was sent while issue was closed.
Description was changed from ========== [base/files] Respect MAC_CHROMIUM_TMPDIR instead of TMPDIR on macOS. Some folks are currently setting $TMPDIR to very long values on macOS, which causes socket name length issues when using $TMPDIR. R=mattm@chromium.org BUG=698759, 696803 ========== to ========== [base/files] Respect MAC_CHROMIUM_TMPDIR instead of TMPDIR on macOS. Some folks are currently setting $TMPDIR to very long values on macOS, which causes socket name length issues when using $TMPDIR. R=mattm@chromium.org BUG=698759, 696803 Review-Url: https://codereview.chromium.org/2770753006 Cr-Commit-Position: refs/heads/master@{#461515} Committed: https://chromium.googlesource.com/chromium/src/+/16abc4a0f8b419fe03dcfba8799c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/16abc4a0f8b419fe03dcfba8799c...
Message was sent while issue was closed.
LGTM still from me too, for whatever you may still need that for.
Message was sent while issue was closed.
On 2017/04/03 17:31:00, iannucci wrote: > On 2017/04/03 17:25:22, Devlin wrote: > https://codereview.chromium.org/2770753006/diff/100001/chrome/browser/extensi... > > File chrome/browser/extensions/test_extension_dir.cc (right): > > > > > https://codereview.chromium.org/2770753006/diff/100001/chrome/browser/extensi... > > chrome/browser/extensions/test_extension_dir.cc:70: return > > base::MakeAbsoluteFilePath(dir_.GetPath()); > > Can we add a comment explaining why this is important? > > Added a comment, but please take a look to review it for accuracy+helpfulness... > I'm new in this code :) retroactive slgtm. :) |
