Skip to content

[Bug]: [Bazel 7?] Fix/document incompatibility with --experimental_inprocess_symlink_creation (transitive resolution failures / sandbox escape) #1877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
adamscybot opened this issue Aug 6, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@adamscybot
Copy link

adamscybot commented Aug 6, 2024

What happened?

I have documented the very confusing effects of this problem just an hour or so ago on #1546 (comment). Very shortly after that, I identified this flag as the problem and a week of debugging comes to a close!

I am unsure if this is related to rules_js 2.x, or Bazel 7 and what mix of these might trigger this problem. Primarily because this is a part of a large simultaneous upgrade. But I can say it happens on rules_js 2.0.0-rc9 combined with Bazel 7.

I feel it is more likely related to a change in Bazel 7 and may apply to older versions of rules_js too. This change to the --experimental_inprocess_symlink_creation flags behaviour was made in Bazel 7.0. See the thread for more details. However, this is speculation. In the interest of caution, I have added a note on that thread about this issue as its being proposed to turn this flag on by default in Bazel 7.3.0.

In my case the problem manifests in the following situation, but it is doubtful to be limited to just this:

  1. --experimental_inprocess_symlink_creation is enabled.
  2. A PNPM workspace with an application package with depends on another internal workspace package.
  3. The internal workspace package has many runtime deps in its package.json. These are also declared on the data attribute of the npm_package.
  4. In the app package.json there is a workspace:* dependency to the shared package.
  5. In the app package, there are js_binary targets connected to js_library targets. Those js_library targets specify the :node_modules/@example/shared-pkg in their data attributes.

Due to --experimental_inprocess_symlink_creation, symlinks are created differently. This happens all the way down the dependency tree in node_modules and I've also seen sandbox escape back to the source files in certain configurations.

This can be seen by executing commands along the lines of (masked some info)

ls -la /private/var/tmp/_bazel_xxxxxxxx/c84d9fb447025e8f818c667552514689/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/xxx-workspace-root-parent-xxx/xxx-workspace-root-xxx/xxx-app-package-xxx/bin_/bin.runfiles/_main/xxx-workspace-root-parent-xxx/xxx-workspace-root-xxx/node_modules/.aspect_rules_js/[email protected]/node_modules

With --experimental_inprocess_symlink_creation turned on, symlinks look like this (snippet + masked):

axios -> /private/var/tmp/_bazel_xxxxxxxx/c84d9fb447025e8f818c667552514689/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/ui-plane/xxx-workspace-root-xxx/node_modules/.aspect_rules_js/[email protected]/node_modules/axios
body-parser -> /private/var/tmp/_bazel_xxxxxxxx/c84d9fb447025e8f818c667552514689/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/ui-plane/xxx-workspace-root-xxx/node_modules/.aspect_rules_js/[email protected]/node_modules/body-parser
combined-stream -> /private/var/tmp/_bazel_xxxxxxxx/c84d9fb447025e8f818c667552514689/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/ui-plane/xxx-workspace-root-xxx/node_modules/.aspect_rules_js/[email protected]/node_modules/combined-stream
connect-redis -> /private/var/tmp/_bazel_xxxxxxxx/c84d9fb447025e8f818c667552514689/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/ui-plane/xxx-workspace-root-xxx/node_modules/.aspect_rules_js/[email protected]/node_modules/connect-redis
cookie-parser -> /private/var/tmp/_bazel_xxxxxxxx/c84d9fb447025e8f818c667552514689/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/ui-plane/xxx-workspace-root-xxx/node_modules/.aspect_rules_js/[email protected]/node_modules/cookie-parser

With --experimental_inprocess_symlink_creation turned off, symlinks look like this (snippet):

axios -> ../../[email protected]/node_modules/axios
body-parser -> ../../[email protected]/node_modules/body-parser
combined-stream -> ../../[email protected]/node_modules/combined-stream
connect-redis -> ../../[email protected]/node_modules/connect-redis
cookie-parser -> ../../[email protected]/node_modules/cookie-parser

The latter works great. The former does not, throwing lots of module not found errors. I beleive it is considered a sandbox escape by the node fs patches.

Version

Development (host) and target OS/architectures: MacOSX 14.5

Output of bazel --version: 7.2.1

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:

bazel_dep(name = "rules_nodejs", version = "6.2.0")

node = use_extension("@rules_nodejs//nodejs:extensions.bzl", "node", dev_dependency = True)
node.toolchain(node_version = "20.11.1")


bazel_dep(name = "aspect_bazel_lib", version = "2.7.8")
bazel_dep(name = "aspect_rules_js", version = "2.0.0-rc9")

Language(s) and/or frameworks involved: Typescript, Javascript, PNPM workspaces

How to reproduce

See above but more details and musing can be found at #1546 (comment).

Ensure build --experimental_inprocess_symlink_creation is in your .bazelrc.

Any other information?

I can happily no longer use this flag, which I am doing now. But it should be blocked/documented or fixed.

I also found it can be worked around (if you for some reason must keep this flag) by using patch_node_fs = False on js_binary but thats probably very dangerous, as like I said, I've actually seen symlinks at times linking out to the src tree.

Debugging the problem has been hard because once the symlinks are bad like this, the behaviour of future ops/changes whilst the flag remains on seems to be non-hermetic and you need to rm -rf some stuff in bazel-out before having another workaround attempt since doing so "changes the errors". I was trying crazy stuff like shoving the workspace node_modules into the js_binary's data attribute. But of course, I now know any "workaround" is futile.

Also I don't know enough about the matter at hand to say, but its quite possible this issue is with Bazel 7 and there is nothing to fix here. If so, I'm happy to move this there.

Side point, but this would of been so much easier for me to realise if this type of "module not found" errors coming from node were supplemented by a an additional message/log (in some kinda of debug mode?) to say they would have resolved if not for the fs patch and why they were rejected/where it expected stuff to be contained within. I'm guessing that the symlinks go to an absolute path and not a relative one is possibly a key invariant that could be stated. As without this realisation, the paths often look plausible.

Note this flag is actually mentioned by aspect as one to (possibly) enable:

https://docs.aspect.build/guides/bazelrc/#notes
https://blog.aspect.build/bazelrc-flags

@adamscybot adamscybot added the bug Something isn't working label Aug 6, 2024
@adamscybot adamscybot changed the title [Bug]: [Bazel 7?] [rules_nodejs 2.x?] Fix or document incompatibility with --experimental_inprocess_symlink_creation [Bug]: [Bazel 7?] [rules_nodejs 2.x?] Fix/document incompatibility with --experimental_inprocess_symlink_creation (causes transitive resolution failures) Aug 6, 2024
@adamscybot adamscybot changed the title [Bug]: [Bazel 7?] [rules_nodejs 2.x?] Fix/document incompatibility with --experimental_inprocess_symlink_creation (causes transitive resolution failures) [Bug]: [Bazel 7?] Fix/document incompatibility with --experimental_inprocess_symlink_creation (causes transitive resolution failures) Aug 6, 2024
@adamscybot adamscybot changed the title [Bug]: [Bazel 7?] Fix/document incompatibility with --experimental_inprocess_symlink_creation (causes transitive resolution failures) [Bug]: [Bazel 7?] Fix/document incompatibility with --experimental_inprocess_symlink_creation (transitive resolution failures / sandbox escape) Aug 6, 2024
@fmeum
Copy link
Contributor

fmeum commented Aug 18, 2024

This is a Bazel bug: bazelbuild/bazel#23327

@adamscybot
Copy link
Author

Since the Bazel folks (thanks @fmeum) picked this up and fixed it in Bazel 7.4, I'm going to close :)

devversion added a commit to devversion/angular-cli that referenced this issue Apr 16, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 16, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 16, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 16, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 16, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 29, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 29, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to devversion/angular-cli that referenced this issue Apr 29, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to angular/angular-cli that referenced this issue Apr 30, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).
devversion added a commit to angular/angular-cli that referenced this issue Apr 30, 2025
After throrough investigation and lots of time spent, I figured out why
the `fs` patch didn't resolve paths within the `.runfiles` directory.

The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks
for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.

This is weird because in other projects there are relative symlinks to
the store (this is important for the sandbox escaping FS patches)), as
otherwise the dereferencing is blocked/prevented and we'd never resolve
into a store directory— preventing transitive deps from being properly
resolved.

After figuring this out, I've tracked down the Bazel issue and found a
fix: bazelbuild/bazel@51d7a3c

In addition, there seems a related `rules_js` issue that has the same
investigation results:
aspect-build/rules_js#1877

We should try this out, but keep an eye on Firefox and Chromium. Those
browser artifacts might contain files with spaces. This is incompatible
with the Bazel version we are using, so the inprocess runfile creation
was a workaround for this— If we remove that flag, we might cause issues
for Mac users.

We have a few options I think:

1. drop the flag, and hope it doesn't break any runfile generation for
  browsers. I suspect it won't work...— alternatively try renaming
  browser files.

2. keep the flag for now, and temporarily keep the disabled FS patches
  (needs to be manually specified by users)

3. do solution (2) but once migration allows it, update to Bazel 7— so
  that the bug with inprocess resolution is nicely fixed.

I think (3) is a good plan moving forward and we don't need any runfile
tricks. In Bazel 8, the runfiles with space issue is also resolved—
allowing us to drop the inprocess runfile creation as well (if we want).

(cherry picked from commit b5b6737)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants