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

[breaking change] On Windows refer to special null file as '\\?\nul' rather than 'nul'. #56308

Open
aam opened this issue Jul 24, 2024 · 16 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes os-windows triaged Issue has been triaged by sub team

Comments

@aam
Copy link
Contributor

aam commented Jul 24, 2024

Change Intent

We want to consistently canonicalize all file names using Windows API so that all file, directory operations can consistently enjoy 32k-file name limit.
Caveat of using that API is that it treats nul as regular file name and one has to use \\?\nul to refer to this special file. In posix it's equivalent of '/dev/null'.

So here is the proposal to make this change in dart: File('NUL').writeAsBytesSync(bytes); becomes File(r'\\?\NUL').writeAsBytesSync(bytes);.

Justification

Windows support for long path names itself is breaking, so if Dart enables developers to use long paths then this breaking change is unavoidable.

Issues that are to be addressed by this change are #56125, #55972

See https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats for details on various windows file name formats.

Impact

This only concerns dart running on Windows. It is expected that use of 'nul' is rare if existent at all.
Existing code that reads from/writes to nul will start throwing because nul will be canonicalized to \\?\UNC\nul - file location that can't be read to or written from.

Mitigation

Code that was reading from or writing to nul will start throwing, have to be changed to refer to \\?\nul.

Change Timeline

There change is already implemented, so it can be submitted as soon as it is approved.

Associated CLs

https://dart-review.git.corp.google.com/c/sdk/+/375421

@aam aam added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. os-windows breaking-change-request This tracks requests for feedback on breaking changes labels Jul 24, 2024
@a-siva
Copy link
Contributor

a-siva commented Jul 24, 2024

//cc @itsjustkevin

@Hixie
Copy link
Contributor

Hixie commented Jul 26, 2024

Could we create a lint that detects 'NUL' being passed to some typical path-accepting arguments and flags it?

This behaviour change is theoretically dangerous because it silently changes apps from writing nothing to writing to disk. If what they're writing is sensitive (e.g. passwords, PII) this could have unfortunate consequences that remain undetected for some time.

That said, I don't know why anyone would be writing to the null device in the first place, it's easier to just... not do anything at all. So I have no objection.

@TekExplorer
Copy link

Writing to a null device could be useful if using a package that normally saves to disk, and gives no other option.

That aside, perhaps we could have the exact path "nul" and any other such known reserved files report through an assert that you should use it's fully qualified name, or prefix with ./ for maximum clarity

I believe that should be warning enough, as a lint seems tricky. Perhaps a lint on uri.parse()? Idk.

@itsjustkevin
Copy link
Contributor

@aam please update this issue body to match the breaking change issue template.

@aam
Copy link
Contributor Author

aam commented Jul 26, 2024

@Hixie wrote

it silently changes apps

It's not going to be a slient change - the code will start throwing. The canonicalization function Windows provide that we use(PathAllocCanonicalize) given nul filename produces \\?\unc\nul, which is deemed invalid by _wopen.

@aam
Copy link
Contributor Author

aam commented Jul 26, 2024

@itsjustkevin wrote

@aam please update this issue body to match the breaking change issue template.

Done.

@Hixie
Copy link
Contributor

Hixie commented Jul 26, 2024

It's not going to be a slient change - the code will start throwing.

Oh, cool, ok. In that case never mind, I have no objections at all.

@zanderso
Copy link
Member

Should the name of the null device be a new field on Platform?

@aam
Copy link
Contributor Author

aam commented Jul 29, 2024

Should the name of the null device be a new field on Platform?

My immediate thought is that it's not used frequently enough to warrant having a field on Platform class.
On the other hand if we added a field though, it would help with this

  if (Platform.isWindows) {
    File('\\?\NUL').writeAsBytesSync(bytes);
  } else {
    File('/dev/null').writeAsBytesSync(bytes);
  }

so it becomes this

  Platform.nullDevice.writeAsBytesSync(bytes);

@TekExplorer
Copy link

Sounds like a viable extension getter

@aam
Copy link
Contributor Author

aam commented Jul 30, 2024

Perhaps it actually makes sense to keep the discussion of whether we should introduce convenience fields for null(or some other built-in) device file separate from this breaking change request.

@a-siva
Copy link
Contributor

a-siva commented Aug 7, 2024

@itsjustkevin any updates on this ?

@itsjustkevin
Copy link
Contributor

@vsmenon and @leonsenft please take a looks at this breaking change

@leonsenft
Copy link

lgtm

@mraleph
Copy link
Member

mraleph commented Aug 14, 2024

LGTM from me (to stand in for @vsmenon)

@a-siva a-siva added the triaged Issue has been triaged by sub team label Aug 14, 2024
@itsjustkevin
Copy link
Contributor

Breaking change approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes os-windows triaged Issue has been triaged by sub team
Projects
Status: In review
Development

No branches or pull requests

8 participants