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

file: make _wopen() and _open() able to handle long legacy DOS paths #13512

Closed
wants to merge 1 commit into from

Conversation

sergio-nsk
Copy link
Contributor

@sergio-nsk sergio-nsk commented Apr 30, 2024

Issue: #8361

Perl in C:\Program Files\Git\usr\bin supports long paths w/o any manifest or registry changes. Why not let curl support long paths.

BTW the solution offered by "5.13 long paths are not fully supported on Windows" in the known bugs does not work, because there is no way how to specify //?/ in file://. RFC 8089 excludes such paths \\?\ and \\.\. file://localhost///?/C:/foo or file://localhost/\\?\C:\foo are also invalid URLs.

@github-actions github-actions bot added the tests label Apr 30, 2024
@sergio-nsk sergio-nsk force-pushed the sergey/long-path/1 branch 2 times, most recently from 469dd66 to d2be1b2 Compare April 30, 2024 23:01
@jay
Copy link
Member

jay commented May 1, 2024

prepending \\?\ to a path disables all parsing of the path. like any forward slashes and .. will not be converted. you are assuming that any path starting with the drive letter and a colon will not need any interpretation

@nmoinvaz
Copy link
Contributor

nmoinvaz commented May 1, 2024

@jay that modified path is only passed into _open and _wopen. Are you seeing otherwise? As far as I can tell after it is used by those functions it is no longer used anywhere else.

@jay
Copy link
Member

jay commented May 1, 2024

that modified path is only passed into _open and _wopen

I agree but I'm saying the logic is wrong. You can't assume that if a user specifies a path with a drive letter and colon that it is not going to need some interpretation. c:\foo/bar/../baz is not \\?\c:\foo/bar/../baz it is \\?\c:\foo\baz. you can't prepend \\?\ without first interpreting. Also IIRC \\?\ is only actually effective on the unicode W-suffixed api functions and crt w functions that wrap them (ie UNICODE/_UNICODE builds)

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented May 1, 2024

prepending \\?\ to a path disables all parsing of the path. like any forward slashes and .. will not be converted. you are assuming that any path starting with the drive letter and a colon will not need any interpretation

Right, it disables the path parsing, but it does not matter, the limitation PATH_MAX breaks the file opening before the parse result that can be less than PATH_MAX. Hence, the result is the same - unable to open a file.

The simple code with the existing 259 char path:

#include <fcntl.h>
#include <io.h>
#include <stdio.h>
#include <string.h>

void win_open(const char *path, const char *msg)  {
   int fh1 = _open( path, _O_RDONLY ); // C4996
   if(fh1 == -1) {
      printf("Open %s failed on file ", msg);
      perror("");
   } else {
      printf("Open %s succeeded on input file\n", msg);
      _close(fh1);
   }
}

int main(void) {
   const char *path259_l =   "\\\\?\\C:\\Work\\solid\\third_party\\curl\\tests\\log\\verylooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooongdir\\dir\\..\\verylooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooongfile3206.txt";
   const char *parsed259_l = "\\\\?\\C:\\Work\\solid\\third_party\\curl\\tests\\log\\verylooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooongdir\\verylooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooongfile3206.txt";

   win_open(path259_l + 4, "259 legacy");
   win_open(path259_l, "259 long");

   win_open(parsed259_l + 4, "259 parsed legacy");
   win_open(parsed259_l, "259 parsed long");
}

The output:

Open 259 legacy failed on file No such file or directory
Open 259 long failed on file Invalid argument
Open 259 parsed legacy succeeded on input file
Open 259 parsed long succeeded on input file

If the unparsed path in the argument exceeds 259 chars, it does not matter, neither that path, nor \\?\... path can be opened. So, \\?\ does not make it worse.

@sergio-nsk
Copy link
Contributor Author

As for forward slashes, curl translates them to backward slashes before curlx_win32_open() call.

@jay
Copy link
Member

jay commented May 1, 2024

Right, it disables the path parsing, but it does not matter, the limitation PATH_MAX breaks the file opening before the parse result that can be less than PATH_MAX. Hence, the result is the same - unable to open a file.

It does matter. I think it's wrong to prepend \\?\ without properly converting the path. Your logic assumes a long path (wcslen(filename_w) >= MAX_PATH) you can just prepend \\?\ and that is not necessarily true. Also IIRC there are still places in curl where we use PATH_MAX/MAX_PATH, as I mentioned in that other issue. You can't assume that all of libcurl code will successfully pass a long path to curlx_win32_open.

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented May 1, 2024

that is not necessarily true.

You can't assume that all of libcurl code will successfully pass a long path to curlx_win32_open.

Not necessarily true, right. I don't assume anything. I just made it better in some cases, and not worse in other cases.

file://localhost/c:/very-long-path and file:///c:/very-long-path now work and this is good for curl.

@bagder bagder added the Windows Windows-specific label May 1, 2024
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

The new test fails in several windows CI jobs.

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented May 3, 2024

Windows builds don't seem to translate / to \ file paths. Will loot for the best solution, maybe just will just replace / with \ in the changed function. Weird, it worked locally at me.

jay added a commit to jay/curl that referenced this pull request May 3, 2024
- Add a helper function for the Windows file wrapper functions that will
  normalize a long path (or a filename in a long path) and add the
  prefix `\\?\` so that Windows will access the file.

Prior to this change if a filename (when normalized internally by
Windows to its full path) or a path was longer than MAX_PATH (260) then
Windows would not open the path, unless it was already normalized by the
user and had the `\\?\` prefix prepended.

The `\\?\` prefix could not be passed to file:// so for example
something like file://c:/foo/bar/filename255chars could not be opened
prior to this change.

There's some code in tool_doswin that will need to be modified as well
to further remove MAX_PATH (aka PATH_MAX) limitation.

Ref: curl#8361
Ref: curl#13512
Ref: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats
Ref: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation

Closes #xxxx
@jay
Copy link
Member

jay commented May 3, 2024

I'm proposing #13522 to address this issue with a normalization check and cover all the file functions that we wrap.

@sergio-nsk
Copy link
Contributor Author

I will check #13522 in WinRT on Monday. Long paths are usual in WinRT.

@sergio-nsk
Copy link
Contributor Author

Closing in favor of the other PR.

@sergio-nsk sergio-nsk closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

4 participants