Skip to content
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

fix: make getChildEntry recursively traverse the path #301

Merged
merged 2 commits into from
Dec 8, 2023
Merged

fix: make getChildEntry recursively traverse the path #301

merged 2 commits into from
Dec 8, 2023

Conversation

whitequark
Copy link
Contributor

I'm testing JCO together with YoWASP. I componentized a regular build of YoWASP Yosys using jco new --wasi-command and jco transpile, and am running it with this script:

import { run } from './yosys.command.js';
import { _setArgs } from '@bytecodealliance/preview2-shim/cli';
import { _setFileData } from '@bytecodealliance/preview2-shim/filesystem';

var fileData = {
    dir: {
        "inv.v": {
            source: "module inv(input i, output o); assign o = ~i; endmodule"
        }
    }
};
_setFileData(fileData)
_setArgs(["yosys", "-Q", "./inv.v", "-o", "inv.il"])
run.run()

console.log(new TextDecoder().decode(fileData.dir["inv.il"].source))

Without this PR, Yosys attempting to open ./inv.v opens the directory (and then crashes because it's not a file). I'm not entirely sure what the intent for the getChildEntry function is, but it seems wrong as-is, and the script completes with my changes.

@guybedford guybedford merged commit ed6c88b into bytecodealliance:main Dec 8, 2023
6 checks passed
@@ -37,15 +37,18 @@ function getChildEntry (parentEntry, subpath, openFlags) {
let segmentIdx;
do {
if (!entry || !entry.dir) throw 'not-directory';
segmentIdx = subpath.indexOf('/');
segmentIdx = subpath.indexOf('/', segmentIdx);
Copy link

@bakkot bakkot Dec 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by: this will search starting at the previous segmentIdx, but that value was an offset into subpath as it was before the later subpath = subpath.substring(segmentIdx + 1) line. Probably you don't want to have both that line and this one?

Consider running this code on longDirName/short/x - the second time through the loop it will start searching the string "short/x" at index 11 (which is past the end of the string), and so miss the / between "short" and "x".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, here's the traverse function I eventually ended up writing, which I think is a lot more correct:

  traverse(path, flags = { create: false, remove: false }) {
    let entry = this;
    let separatorAt = -1;
    do {
      if (entry instanceof File)
        throw 'not-directory';
      const files = entry.files;
      separatorAt = path.indexOf('/');
      const segment = separatorAt === -1 ? path : path.substring(0, separatorAt);
      if (separatorAt === -1 && flags.remove)
        delete files[segment];
      else if (segment === '' || segment === '.')
        /* disregard */;
      else if (segment === '..')
        /* hack to make scandir() work */;
      else if (Object.hasOwn(files, segment))
        entry = files[segment];
      else if (flags.create === 'directory' || flags.create === 'file' && separatorAt !== -1)
        entry = files[segment] = new Directory({});
      else if (flags.create === 'file')
        entry = files[segment] = new File(new Uint8Array());
      else
        throw 'no-entry';
      path = path.substring(separatorAt + 1);
    } while (separatorAt !== -1);
    return entry;
  }

It's not quite compatible with this specific shim but I think the algorithm is sound.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing this one @bakkot, the browser implementation is considered experimental and is kinda on the verge of either deprecation or heavy refactoring depending on which way we go here. I do like the idea of refining this implementation into something more like what was discussed in #303 (comment).

For now, it's kinda still placeholder code and lots of code paths are entirely untested, a decision will be made on the deprecation / refactoring soon though so it won't be staying in this state.

For now I posted a follow-up in #309. Any review there would be very welcome. Always fun to have the opportunity to use a bodiless conditional...

@whitequark whitequark deleted the patch-2 branch December 9, 2023 04:29
guybedford added a commit that referenced this pull request Dec 11, 2023
@guybedford guybedford mentioned this pull request Dec 11, 2023
guybedford added a commit that referenced this pull request Dec 14, 2023
* Revert "fix: make getChildEntry recursively traverse the path (#301)"

This reverts commit ed6c88b.

* fix: possible browser traversal fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants