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

realpathSync('/') returns invalid result (edge case bug) #863

Closed
williamstein opened this issue Oct 25, 2022 · 3 comments · Fixed by #867
Closed

realpathSync('/') returns invalid result (edge case bug) #863

williamstein opened this issue Oct 25, 2022 · 3 comments · Fixed by #867
Labels

Comments

@williamstein
Copy link
Contributor

This is easy to reproduce:

> memfs = require('memfs');
> memfs.vol.fromJSON({'./a':'a'}, '/');
> memfs.vol.realpathSync('/')
''

Notice that the output is an empty string, which isn't a valid path. This bug is caused by getPath() in node.ts:

  getPath(): string {
    return this.steps.join(SEP);
  }

The problem is the edge case of join when this.steps is [''], i.e., a list with a single empty string.

The following is what I would expect as output in this case:

> require('fs').realpathSync('/')
'/'

Perhaps a good fix would be:

  getPath(): string {
    const path = this.steps.join(SEP);
    return path ? path : "/";
  }
@williamstein williamstein changed the title realpathSync('/') returns incorrect and invalid result (edge case bug) realpathSync('/') returns invalid result (edge case bug) Oct 25, 2022
@williamstein
Copy link
Contributor Author

My suggestion above breaks many tests. Here's an alternative that fixes this issue, and includes a jest test too (which passes):

diff --git a/src/__tests__/volume/realpathSync.test.ts b/src/__tests__/volume/realpathSync.test.ts
index 58d7746..251dfb3 100644
--- a/src/__tests__/volume/realpathSync.test.ts
+++ b/src/__tests__/volume/realpathSync.test.ts
@@ -1,6 +1,6 @@
 import { create } from '../util';
 
-describe('.realpath(...)', () => {
+describe('.realpathSync(...)', () => {
   it('works with symlinks, #463', () => {
     const vol = create({});
     vol.mkdirSync('/a');
@@ -12,3 +12,8 @@ describe('.realpath(...)', () => {
     expect(path).toBe('/c/index.js');
   });
 });
+
+describe("edge case -- realpathSync('/') returns '/'", () => {
+  const vol = create({'./a':'a'});
+  expect(vol.realpathSync('/')).toBe('/');
+});
\ No newline at end of file
diff --git a/src/volume.ts b/src/volume.ts
index 4f63532..b34860e 100644
--- a/src/volume.ts
+++ b/src/volume.ts
@@ -1493,7 +1493,8 @@ export class Volume {
     const realLink = this.getResolvedLink(steps);
     if (!realLink) throw createError(ENOENT, 'realpath', filename);
 
-    return strToEncoding(realLink.getPath(), encoding);
+    const path = realLink.getPath();
+    return strToEncoding(path ? path : "/", encoding);
   }
 
   realpathSync(path: PathLike, options?: IRealpathOptions | string): TDataOut {

williamstein added a commit to sagemathinc/memfs-js that referenced this issue Oct 26, 2022
@G-Rath
Copy link
Collaborator

G-Rath commented Oct 26, 2022

@williamstein how would you feel about doing a PR? :)

williamstein added a commit to williamstein/memfs that referenced this issue Oct 26, 2022
G-Rath added a commit that referenced this issue Nov 13, 2022
* fix #863 -- realpathSync('/') returns invalid result

-  #863

* run prettier

* style: move a test case

* refactor: inline conditional

Co-authored-by: Gareth Jones <Jones258@Gmail.com>
streamich pushed a commit that referenced this issue Nov 13, 2022
## [3.4.11](v3.4.10...v3.4.11) (2022-11-13)

### Bug Fixes

* return `/` when calling `realpathSync` with `/` ([#867](#867)) ([8d8e8d1](8d8e8d1)), closes [#863](#863)
@streamich
Copy link
Owner

🎉 This issue has been resolved in version 3.4.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants