dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.06k stars 1.56k forks source link

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

Open aam opened 1 month ago

aam commented 1 month ago

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 https://github.com/dart-lang/sdk/issues/56125, https://github.com/dart-lang/sdk/issues/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

a-siva commented 1 month ago

//cc @itsjustkevin

Hixie commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

aam commented 1 month ago

@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 commented 1 month ago

@itsjustkevin wrote

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

Done.

Hixie commented 1 month ago

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 commented 1 month ago

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

aam commented 1 month ago

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 commented 1 month ago

Sounds like a viable extension getter

aam commented 1 month ago

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 commented 1 month ago

@itsjustkevin any updates on this ?

itsjustkevin commented 1 month ago

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

leonsenft commented 1 month ago

lgtm

mraleph commented 3 weeks ago

LGTM from me (to stand in for @vsmenon)

itsjustkevin commented 3 weeks ago

Breaking change approved