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

CONOUT$ no longer available for Node 22? #54161

Open
2 tasks done
jpage-godaddy opened this issue Jul 10, 2024 · 17 comments
Open
2 tasks done

CONOUT$ no longer available for Node 22? #54161

jpage-godaddy opened this issue Jul 10, 2024 · 17 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.

Comments

@jpage-godaddy
Copy link
Contributor

jpage-godaddy commented Jul 10, 2024

Node.js Version

v22.4.1

NPM Version

v10.8.1

Operating System

Windows 11

Subsystem

fs, process

Description

I am trying to fix some code for compatibility with node 22 that writes directly to the terminal. For unix-like OS's, things work fine due to /dev/tty, but for Windows, the code uses process.binding('fs') hackery to open the conout$ magic file. However, this no-longer works in node 22, which treats conout$ as a regular filename and of course cannot find it.

Is there a way to make something like this work for node 22? Is there finally a cleaner option to writing to the Windows console directly without using stdout?

const openWindowsConsole = () => {
  // vX.X.X
  const nodeVersion = parseInt(process.version.split('.')[0].slice(1), 10);

  // For node 22, we seem to be unable to open CONOUT$
  if (nodeVersion >= 22) {
    // What do we do here?
    throw new Error('This application is incompatible with Windows & node 22');
  }

  const openArgs = [
    "CONOUT$",
    // eslint-disable-next-line no-bitwise
    fs.constants.O_RDWR | fs.constants.O_EXCL,
    0o666
  ];

  if (nodeVersion < 20) {
    // Older version of node require null being passed explicitly for the
    // optional arguments, but this breaks node 20.
    openArgs.push(null, null)
  }

  return new tty.WriteStream(
    process.binding("fs").open(...openArgs)
  );
}

Minimal Reproduction

Using node 22, on Windows run this code:

const fs = require('fs');
process.binding("fs").open('conout$', fs.constants.O_RDWR | fs.constants.O_EXCL, 0o666);

Output

Uncaught [Error: ENOENT: no such file or directory, open 'C:\Users\jpage\conout$'] {
  errno: -4058,
  code: 'ENOENT',
  syscall: 'open',
  path: 'C:\\Users\\jpage\\conout$'
}

Before You Submit

  • I have looked for issues that already exist before submitting this
  • My issue follows the guidelines in the README file, and follows the 'How to ask a good question' guide at https://stackoverflow.com/help/how-to-ask
@RedYetiDev
Copy link
Member

@nodejs/fs PTAL

@RedYetiDev RedYetiDev added the windows Issues and PRs related to the Windows platform. label Jul 10, 2024
@lpinca
Copy link
Member

lpinca commented Aug 1, 2024

I hit the same issue. I think it makes sense to move this to nodejs/issues.

@lpinca lpinca transferred this issue from nodejs/help Aug 1, 2024
@lpinca lpinca added the fs Issues and PRs related to the fs subsystem / file system. label Aug 1, 2024
@lpinca
Copy link
Member

lpinca commented Aug 1, 2024

The minimal test case passes on Node.js 22.3.0 but fails on Node.js 22.4.0 I think the culprit is #52135.

@anonrig can you take a look?

@lpinca
Copy link
Member

lpinca commented Aug 1, 2024

On second look I do not think it is #52135, it does not seem to touch process.bindings('fs').open(). I'll run git bisect tomorrow.

Edit: it does.

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

Works on 2eff28f, fails on 399eb33.

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

The following patch cannot land but fixes the issue.

diff --git a/src/node_file.cc b/src/node_file.cc
index a86482b194..0cc5a821cd 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -2137,7 +2137,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(env->isolate(), args[0]);
   CHECK_NOT_NULL(*path);
-  ToNamespacedPath(env, &path);
+  // ToNamespacedPath(env, &path);
 
   CHECK(args[1]->IsInt32());
   const int flags = args[1].As<Int32>()->Value();

#52135 should have been marked as semver-major.

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

I think there is currently no workaround. fs.openSync('\\\\.\\CON', 'w') also fails because a trailing \ is added by ToNamespacedPath().

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

cc: @nodejs/platform-windows @huseyinacacak-janea

@targos
Copy link
Member

targos commented Aug 2, 2024

#52135 should have been marked as semver-major.

I don't understand why you say that. The PR was supposed to be only refactoring. If it changed the behavior on Windows, it should be fixed or reverted.

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

I don't understand why you say that. The PR was supposed to be only refactoring. If it changed the behavior on Windows, it should be fixed or reverted.

The problem is that userland modules uses process.binding('fs').open() directly and #52135 changed it in a breaking way.

@targos
Copy link
Member

targos commented Aug 2, 2024

process.binding is out of semver guarantees.

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

Fair enough. I wonder if the trailing slash added by path.win32.resolve() and node::PathResolve() is needed. There are only 3 tests (easy to fix) failing with this patch that would at least make #54161 (comment) viable.

diff --git a/lib/path.js b/lib/path.js
index f72f5835e9..ce91895071 100644
--- a/lib/path.js
+++ b/lib/path.js
@@ -320,7 +320,7 @@ const win32 = {
                                    isPathSeparator);
 
     return resolvedAbsolute ?
-      `${resolvedDevice}\\${resolvedTail}` :
+      `${resolvedDevice}${resolvedTail ? '\\' + resolvedTail : ''}` :
       `${resolvedDevice}${resolvedTail}` || '.';
   },
 
diff --git a/src/path.cc b/src/path.cc
index fade21c8af..98b420d55a 100644
--- a/src/path.cc
+++ b/src/path.cc
@@ -221,7 +221,11 @@ std::string PathResolve(Environment* env,
   resolvedTail = NormalizeString(resolvedTail, !resolvedAbsolute, "\\");
 
   if (resolvedAbsolute) {
-    return resolvedDevice + "\\" + resolvedTail;
+    if (!resolvedTail.empty()) {
+      return resolvedDevice + "\\" + resolvedTail;
+    }
+
+    return resolvedDevice;
   }
 
   if (!resolvedDevice.empty() || !resolvedTail.empty()) {

@lpinca
Copy link
Member

lpinca commented Aug 2, 2024

The above patch breaks the following expectation

path.win32.toNamespacedPath('c://') === '\\\\?\\c:\\';

I'm out of ideas.

@anonrig
Copy link
Member

anonrig commented Aug 5, 2024

I'll take a look, but it seems like the change fixed a bypass. We can add a specific bypass to ToNamespacedPath for this value?

@huseyinacacak-janea
Copy link
Contributor

I think there is currently no workaround. fs.openSync('\\\\.\\CON', 'w') also fails because a trailing \ is added by ToNamespacedPath().

@lpinca I'm working on fixing this issue. It is very similar to #54025

@lpinca
Copy link
Member

lpinca commented Aug 6, 2024

I've also created https://github.com/lpinca/fs-open-sync.

@EarlyRiser42
Copy link
Contributor

I’ve submitted a PR to fix the issue at #54367. If anyone has a moment, I would greatly appreciate your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

7 participants