[FS] Fix resolvePath handling of .. up from fs mountpoint #23990

hoodmane · 2025-03-26T13:46:21Z

node.parent doesn't actually give us the parent if the node is a FS root. In that case, instead of using node.parent we should start over from the beginning.

Resolves #23983.

node.parent doesn't actually give us the parent if the node is a FS root. In that case, instead of using `node.parent` we should start over from the beginning. Resolves emscripten-core#23983.

sbc100 · 2025-03-26T14:25:10Z

src/lib/libfs.js

@@ -199,7 +199,12 @@ FS.staticInit();

          if (parts[i] === '..') {
            current_path = PATH.dirname(current_path);
-            current = current.parent;
+            if (FS.isRoot(current)) {
+              path = current_path + '/' + parts.slice(i + 1).join('/');


If the current node is the root node can we just assume current path is / here and do path = '/' + parts.slice(i + 1).join('/')

No, because FS.isRoot() returns true if it's the root of a mount. So in the test, because we mounted NODEFS at /working, FS.isRoot() will return true when processing the .. in the path "/working/../other".

sbc100 · 2025-03-26T14:26:41Z

src/lib/libfs.js

+              path = current_path + '/' + parts.slice(i + 1).join('/');
+              continue linkloop;
+            } else {
+              current = current.parent;


If parts[i] === '..' and the current not is the FS root shouldn't we be failing? (Since there is no parent of the root?)

This is not consistent with linux's stat. If a .. is seen at the root, it is ignored:

$ stat /../lib File: /../lib -> usr/lib Size: 7 Blocks: 0 IO Block: 4096 symbolic link

This reverts commit ee76d83.

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (44) test expectation files were updated by running the tests with `--rebaseline`: ``` code_size/random_printf_wasm.json: 12494 => 12490 [-4 bytes / -0.03%] code_size/random_printf_wasm2js.json: 17262 => 17261 [-1 bytes / -0.01%] other/codesize/test_codesize_cxx_ctors1.gzsize: 8219 => 8232 [+13 bytes / +0.16%] other/codesize/test_codesize_cxx_ctors1.jssize: 19929 => 19993 [+64 bytes / +0.32%] other/codesize/test_codesize_cxx_ctors1.size: 129206 => 129195 [-11 bytes / -0.01%] other/codesize/test_codesize_cxx_ctors2.gzsize: 8207 => 8221 [+14 bytes / +0.17%] other/codesize/test_codesize_cxx_ctors2.jssize: 19907 => 19971 [+64 bytes / +0.32%] other/codesize/test_codesize_cxx_ctors2.size: 128599 => 128588 [-11 bytes / -0.01%] other/codesize/test_codesize_cxx_except.gzsize: 9217 => 9231 [+14 bytes / +0.15%] other/codesize/test_codesize_cxx_except.jssize: 23666 => 23730 [+64 bytes / +0.27%] other/codesize/test_codesize_cxx_except.size: 170827 => 170865 [+38 bytes / +0.02%] other/codesize/test_codesize_cxx_except_wasm.gzsize: 8165 => 8177 [+12 bytes / +0.15%] other/codesize/test_codesize_cxx_except_wasm.jssize: 19821 => 19885 [+64 bytes / +0.32%] other/codesize/test_codesize_cxx_except_wasm.size: 144582 => 144571 [-11 bytes / -0.01%] other/codesize/test_codesize_cxx_except_wasm_legacy.gzsize: 8165 => 8177 [+12 bytes / +0.15%] other/codesize/test_codesize_cxx_except_wasm_legacy.jssize: 19821 => 19885 [+64 bytes / +0.32%] other/codesize/test_codesize_cxx_except_wasm_legacy.size: 142157 => 142146 [-11 bytes / -0.01%] other/codesize/test_codesize_cxx_lto.gzsize: 8234 => 8245 [+11 bytes / +0.13%] other/codesize/test_codesize_cxx_lto.jssize: 20003 => 20067 [+64 bytes / +0.32%] other/codesize/test_codesize_cxx_lto.size: 121939 => 121914 [-25 bytes / -0.02%] other/codesize/test_codesize_cxx_mangle.gzsize: 9255 => 9268 [+13 bytes / +0.14%] other/codesize/test_codesize_cxx_mangle.jssize: 23780 => 23844 [+64 bytes / +0.27%] other/codesize/test_codesize_cxx_mangle.size: 232604 => 232689 [+85 bytes / +0.04%] other/codesize/test_codesize_cxx_noexcept.gzsize: 8219 => 8232 [+13 bytes / +0.16%] other/codesize/test_codesize_cxx_noexcept.jssize: 19929 => 19993 [+64 bytes / +0.32%] other/codesize/test_codesize_cxx_noexcept.size: 131769 => 131758 [-11 bytes / -0.01%] other/codesize/test_codesize_cxx_wasmfs.size: 169129 => 169165 [+36 bytes / +0.02%] other/codesize/test_codesize_files_js_fs.gzsize: 7528 => 7542 [+14 bytes / +0.19%] other/codesize/test_codesize_files_js_fs.jssize: 18487 => 18551 [+64 bytes / +0.35%] other/codesize/test_codesize_files_wasmfs.size: 49968 => 50038 [+70 bytes / +0.14%] other/codesize/test_codesize_hello_O0.size: 15170 => 15166 [-4 bytes / -0.03%] other/codesize/test_codesize_hello_O1.size: 2673 => 2671 [-2 bytes / -0.07%] other/codesize/test_codesize_hello_O2.size: 1977 => 1975 [-2 bytes / -0.10%] other/codesize/test_codesize_hello_O3.size: 1737 => 1735 [-2 bytes / -0.12%] other/codesize/test_codesize_hello_Os.size: 1728 => 1726 [-2 bytes / -0.12%] other/codesize/test_codesize_hello_Oz.size: 1263 => 1261 [-2 bytes / -0.16%] other/codesize/test_codesize_hello_dylink.size: 8176 => 8174 [-2 bytes / -0.02%] other/codesize/test_codesize_hello_single_file.gzsize: 3690 => 3694 [+4 bytes / +0.11%] other/codesize/test_codesize_hello_single_file.jssize: 6783 => 6779 [-4 bytes / -0.06%] other/codesize/test_codesize_hello_wasmfs.size: 1737 => 1735 [-2 bytes / -0.12%] other/codesize/test_codesize_minimal_pthreads.size: 19396 => 19394 [-2 bytes / -0.01%] other/test_unoptimized_code_size.wasm.size: 15170 => 15166 [-4 bytes / -0.03%] other/test_unoptimized_code_size_no_asserts.wasm.size: 12251 => 12247 [-4 bytes / -0.03%] other/test_unoptimized_code_size_strict.wasm.size: 15170 => 15166 [-4 bytes / -0.03%] Average change: +0.08% (-0.16% - +0.35%) ```

hoodmane · 2025-03-27T15:36:15Z

I'm still having a lot of trouble with the "Check Expectations" job...

sbc100 · 2025-03-27T16:18:32Z

I'm still having a lot of trouble with the "Check Expectations" job...

You did ./emsdk install tot and ./tools/maint/rebaseline_tests.py

hoodmane · 2025-03-27T16:34:55Z

Yes. I haven't sourced emsdk_env.sh but in .emscripten LLVM_ROOT and BINARYEN_ROOT point into emsdk/upstream.

hoodmane · 2025-03-27T16:39:20Z

Well I went ahead and manually applied the changes reported in the CI.

kleisauke · 2025-03-27T16:59:52Z

I'm still having a lot of trouble with the "Check Expectations" job...

FWIW, you're not the only one. The *.gzsize code sizes always cause slight differences when I run tools/maint/rebaseline_tests.py on my Fedora 41 workstation, likely because zlib was replaced with zlib-ng. 😅

kleisauke · 2025-03-27T17:07:46Z

I went ahead and manually applied the changes reported in the CI.

It looks like you need to replace the old value instead of appending the new value (i.e. there should be only one newline in these files).

hoodmane · 2025-03-27T17:20:55Z

Oops.

sbc100 · 2025-03-27T17:28:28Z

Yes. I haven't sourced emsdk_env.sh but in .emscripten LLVM_ROOT and BINARYEN_ROOT point into emsdk/upstream.

That should be enough, yes. As long as emsdk/upstream matches emsdk install tot. You can also check that the version of the SDK that emsdk install tot installs for you locally matches the one that is used in the CI.

For example, it should say something like Installing SDK 'sdk-releases-5f2af795eae65fbcb0ebf4c5f1b460fe288f05c9-64bit'..

If you cannot get your local results to match that is certainly worth investigating and fixing.

@kleisauke for the zlib issues is there an open ticket? I wonder how we could fix it? We use python's with gzip.open(f_gz, 'wb') as gzf: to generate those file? Perhaps we could switch to something else?

kleisauke · 2025-03-27T18:20:30Z

@kleisauke for the zlib issues is there an open ticket? I wonder how we could fix it? We use python's with gzip.open(f_gz, 'wb') as gzf: to generate those file? Perhaps we could switch to something else?

I still need to open an issue for this. The solution is unclear as gzip compressed data may not be bit-identical across different zlib versions (see also zlib-ng/zlib-ng#905). Commit 995c887 (in PR #23998) demonstrates this.

sbc100 · 2025-03-27T19:16:43Z

@kleisauke for the zlib issues is there an open ticket? I wonder how we could fix it? We use python's with gzip.open(f_gz, 'wb') as gzf: to generate those file? Perhaps we could switch to something else?

I still need to open an issue for this. The solution is unclear as gzip compressed data may not be bit-identical across different zlib versions (see also zlib-ng/zlib-ng#905). Commit 995c887 (in PR #23998) demonstrates this.

Mabye we ship our own zip implementation as a wasm file :)

hoodmane · 2025-03-27T19:36:06Z

Now the failure says:

code_size/random_printf_wasm.json: 12494 => 12494 [+0 bytes / +0.00%]
code_size/random_printf_wasm2js.json: 17262 => 17262 [+0 bytes / +0.00%]

sbc100 · 2025-03-27T20:05:16Z

Now the failure says:

code_size/random_printf_wasm.json: 12494 => 12494 [+0 bytes / +0.00%]
code_size/random_printf_wasm2js.json: 17262 => 17262 [+0 bytes / +0.00%]

Oh I think the report generator doesn't know how to parse the those json files. Thats a bug in the script. If you look further up the output for test_random_printf_wasm you should see an actual difference I think.

sbc100 · 2025-03-27T20:07:22Z

I think maybe the first thing to do is switch back the upstream main branch and try to confirm that you can replicate the current results.

i.e. can you run those tests and see no changes? Since we know that the main branch if up-to-date at this point, if you cannot replicate the results then there must be something up with your config.

Can you perhaps try with export EM_CONFIG=/path/to/emsdk/.emscripten? That is how I recommend configuring emscripten to use emsdk binaries.

hoodmane · 2025-03-27T20:46:34Z

On the main branch I got:

test expectations are up-to-date

I'll try again on this branch.

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (2) test expectation files were updated by running the tests with `--rebaseline`: ``` code_size/random_printf_wasm.json: 12494 => 12494 [+0 bytes / +0.00%] code_size/random_printf_wasm2js.json: 17262 => 17262 [+0 bytes / +0.00%] Average change: +0.00% (+0.00% - +0.00%) ```

hoodmane · 2025-03-27T20:47:17Z

It seems as if export EM_CONFIG=/path/to/emsdk/.emscripten may have helped.

sbc100 · 2025-03-27T21:19:31Z

It seems as if export EM_CONFIG=/path/to/emsdk/.emscripten may have helped.

Perhaps the version of node is also important? I guess it depends what else is in your local .emscripten?

kleisauke · 2025-04-01T19:25:44Z

FWIW, I worked around that zlib issue with this Dockerfile:

Details

# Build and run with:
# docker build -t emscripten --build-arg CACHEBUST=$(date +%s) .
# docker run -v ${PWD}:/src -v ~/.gitconfig:/root/.gitconfig emscripten
FROM docker.io/library/ubuntu:24.04

# Install needed dependencies
RUN \
  apt-get update -qq && \
  apt-get install -qqy --no-install-recommends \
    ca-certificates \
    curl \
    git \
    libatomic1 \
    python3 \
    xz-utils \
    && \
  apt-get clean -y && \
  rm -rf /var/lib/apt/lists/* 

WORKDIR /opt

ARG CACHEBUST=1

# Install emsdk tip-of-tree
ENV EM_CONFIG=/opt/emsdk/.emscripten
RUN \
  git clone https://github.com/emscripten-core/emsdk.git emsdk && \
  cd emsdk && \
  ./emsdk install tot && \
  ./emsdk activate tot && \
  echo "JS_ENGINES = [NODE_JS]" >> $EM_CONFIG

# The emscripten dir is mounted here
WORKDIR /src

CMD ["./tools/maint/rebaseline_tests.py"]

sbc100 · 2025-04-01T20:35:43Z

FWIW, I worked around that zlib issue with this Dockerfile:

Details

# Build and run with:
# docker build -t emscripten --build-arg CACHEBUST=$(date +%s) .
# docker run -v ${PWD}:/src -v ~/.gitconfig:/root/.gitconfig emscripten
FROM docker.io/library/ubuntu:24.04

# Install needed dependencies
RUN \
  apt-get update -qq && \
  apt-get install -qqy --no-install-recommends \
    ca-certificates \
    curl \
    git \
    python3 \
    xz-utils \
    && \
  apt-get clean -y && \
  rm -rf /var/lib/apt/lists/* 

WORKDIR /opt

ARG CACHEBUST=1

# Install emsdk tip-of-tree
ENV EM_CONFIG=/opt/emsdk/.emscripten
RUN \
  git clone https://github.com/emscripten-core/emsdk.git emsdk && \
  cd emsdk && \
  ./emsdk install tot && \
  ./emsdk activate tot && \
  echo "JS_ENGINES = [NODE_JS]" >> $EM_CONFIG

# The emscripten dir is mounted here
WORKDIR /src

CMD ["./tools/maint/rebaseline_tests.py"]

OGM! Impressive solution.

FIX resolvePath handling of .. up from fs root

66fa4f7

node.parent doesn't actually give us the parent if the node is a FS root. In that case, instead of using `node.parent` we should start over from the beginning. Resolves emscripten-core#23983.

hoodmane mentioned this pull request Mar 26, 2025

Relative path lookups don't work anymore in a directory mounted with IDBFS with emscripten 3.1.74 #23983

Closed

sbc100 reviewed Mar 26, 2025

View reviewed changes

sbc100 approved these changes Mar 26, 2025

View reviewed changes

sbc100 changed the title ~~FIX resolvePath handling of .. up from fs root~~ [FS] Fix resolvePath handling of .. up from fs mountpoint Mar 26, 2025

hoodmane added 2 commits March 26, 2025 16:58

Merge branch 'main' into parent-of-fs-root

9001a57

Rebaseline code size

ee76d83

hoodmane force-pushed the parent-of-fs-root branch from 6f3cb88 to ee76d83 Compare March 26, 2025 16:02

hoodmane added 3 commits March 27, 2025 10:06

Revert "Rebaseline code size"

d37abdb

This reverts commit ee76d83.

Merge branch 'main' into parent-of-fs-root

a5a0733

Update code sizes

525ef33

hoodmane enabled auto-merge (squash) March 27, 2025 16:39

Update test sizes again

ecf6080

Merge branch 'main' into parent-of-fs-root

0b5b8aa

hoodmane merged commit 06d5399 into emscripten-core:main Mar 27, 2025
24 checks passed

hoodmane deleted the parent-of-fs-root branch March 28, 2025 09:37

dschuff mentioned this pull request Mar 29, 2025

Revert "Temporarily disable some tests (#23933) #24011

Merged

kleisauke mentioned this pull request May 8, 2025

Fix handling of uint64_t in Embind #24285

Merged

[FS] Fix resolvePath handling of .. up from fs mountpoint #23990

[FS] Fix resolvePath handling of .. up from fs mountpoint #23990

Uh oh!

Conversation

hoodmane commented Mar 26, 2025

Uh oh!

sbc100 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

hoodmane Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

hoodmane Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

sbc100 commented Mar 27, 2025

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

kleisauke commented Mar 27, 2025

Uh oh!

kleisauke commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

sbc100 commented Mar 27, 2025

Uh oh!

kleisauke commented Mar 27, 2025

Uh oh!

sbc100 commented Mar 27, 2025

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

sbc100 commented Mar 27, 2025

Uh oh!

sbc100 commented Mar 27, 2025

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

hoodmane commented Mar 27, 2025

Uh oh!

sbc100 commented Mar 27, 2025

Uh oh!

Uh oh!

kleisauke commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Apr 1, 2025

Uh oh!

Uh oh!

kleisauke commented Mar 27, 2025 •

edited

Loading

kleisauke commented Apr 1, 2025 •

edited

Loading