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

curl_multibyte: support Windows paths longer than MAX_PATH #13522

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

jay
Copy link
Member

@jay jay commented 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: #8361
Ref: #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

lib/curl_multibyte.c Outdated Show resolved Hide resolved
lib/curl_multibyte.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Jul 1, 2024

Merge conflicts. Any plans to move forward on this?

@nmoinvaz
Copy link
Contributor

nmoinvaz commented Jul 2, 2024

It is still necessary for our use case. @jay can you take a look at the conflicts?

@jay jay force-pushed the multibyte_long_path branch from 78bffc7 to 518204d Compare July 19, 2024 16:51
@jay
Copy link
Member Author

jay commented Jul 19, 2024

I fixed the conflicts. Also I reversed and took @nmoinvaz's suggestion to use 'goto cleanup' instead of 'goto error' to eliminate any confusion in the logic.

Though I don't foresee this PR causing any problems I'm going to wait until the next feature window to land it.

@jay jay force-pushed the multibyte_long_path branch from 518204d to 2250ebf Compare July 19, 2024 17:43
@jay jay force-pushed the multibyte_long_path branch from 2250ebf to 66d5128 Compare August 4, 2024 22:47
@nmoinvaz
Copy link
Contributor

@bagder is it possible for this to be merged for the next release? Thanks.

@bagder bagder removed the needs-info label Sep 15, 2024
@bagder
Copy link
Member

bagder commented Sep 15, 2024

@nmoinvaz this PR is done by @jay and he can/will merge this when he thinks it is fine.

@nmoinvaz
Copy link
Contributor

Ok I wasn't sure. I just wanted to make sure it wasn't abandoned because you told us on another PR that you closed it because we didn't push hard enough for it. Thanks!

@jay
Copy link
Member Author

jay commented Sep 16, 2024

This is held up by a CI failure in a Windows UWP job that I haven't had time to reproduce, apparently there is a missing GetFullPathNameW but according to the doc it exists in UWP apps

In file included from D:/a/curl/curl/bld/src/CMakeFiles/curl.dir/Unity/unity_0_c.c:139:
D:/a/curl/curl/lib/curl_multibyte.c: In function 'fix_excessive_path':
D:/a/curl/curl/lib/curl_multibyte.c:150:20: error: implicit declaration of function 'GetFullPathNameW'; did you mean 'GetLongPathNameW'? [-Wimplicit-function-declaration]
  150 |   needed = (size_t)GetFullPathNameW(in_w, 0, NULL, NULL);
      |                    ^~~~~~~~~~~~~~~~
      |                    GetLongPathNameW
D:/a/curl/curl/lib/curl_multibyte.c:150:20: error: nested extern declaration of 'GetFullPathNameW' [-Werror=nested-externs]
cc1.exe: all warnings being treated as errors
ninja: build stopped: subcommand failed.

The earliest this will be in a release is 2 months from now because it's not going in the patch release coming this Wednesday

@sergio-nsk
Copy link
Contributor

sergio-nsk commented Sep 16, 2024

  1. curl defines WIN32_LEAN_AND_MEAN. Thus, #include <fileapi.h> should be in setup-win32.h.
  2. If it does not help then Windows SDK should be upgraded.

@jay jay force-pushed the multibyte_long_path branch from 66d5128 to 98f9ca4 Compare September 26, 2024 17:16
@jay
Copy link
Member Author

jay commented Sep 26, 2024

setup-win32.h is used everywhere and we purposely use WIN32_LEAN_AND_MEAN so that all the extra stuff is not included in every unit. fileapi.h should be included because the file api is included by winbase which is included by windows.h regardless of lean and mean. not all sdks have the file api in fileapi.h. possibly this is a unity build issue?

@jay
Copy link
Member Author

jay commented Oct 5, 2024

@vszakats do you have any idea about what is causing this UWP CI job failure, before I set up a Windows environment that can build uwp and find out why the ci job is failing, or is there a better way to go into the ci or something without doing all that? AFAICT I'd have to add these packages:

- uses: msys2/setup-msys2@ddf331adaebd714795f1042345e6ca57bd66cea8 # v2
if: ${{ matrix.sys != 'msys' }}
with:
msystem: ${{ matrix.sys }}
install: >-
mingw-w64-${{ matrix.env }}-cc
mingw-w64-${{ matrix.env }}-${{ matrix.build }} ${{ matrix.build == 'autotools' && 'make' || '' }}
openssh
mingw-w64-${{ matrix.env }}-openssl
mingw-w64-${{ matrix.env }}-libssh2
mingw-w64-${{ matrix.env }}-libpsl
mingw-w64-${{ matrix.env }}-c-ares

so

pacman --noconfirm --ask 20 --noprogressbar --sync --needed \
  mingw-w64-x86_64-cc \
  mingw-w64-x86_64-cmake \
  openssh \
  mingw-w64-x86_64-openssl \
  mingw-w64-x86_64-libssh2 \
  mingw-w64-x86_64-libpsl \
  mingw-w64-x86_64-c-ares \
  mingw-w64-x86_64-winstorecompat-git

and then build with the flags from the uwp 'test' configuration

if [ '${{ matrix.test }}' = 'uwp' ]; then
options+=' -DCMAKE_SYSTEM_NAME=WindowsStore -DCMAKE_SYSTEM_VERSION=10.0'
pacman --noconfirm --ask 20 --noprogressbar --sync --needed 'mingw-w64-${{ matrix.env }}-winstorecompat-git'
specs="$(realpath gcc-specs-uwp)"
gcc -dumpspecs | sed -e 's/-lmingwex/-lwindowsapp -lmingwex -lwindowsapp -lwindowsappcompat/' -e 's/-lmsvcrt/-lmsvcr120_app/' > "${specs}"
cflags="-specs=$(cygpath -w "${specs}") -DWINSTORECOMPAT -DWINAPI_FAMILY=WINAPI_FAMILY_APP"
# CMake (as of v3.26.4) gets confused and applies the MSVC rc.exe command-line
# template to windres. Reset it to the windres template manually:
rcopts='<CMAKE_RC_COMPILER> -O coff <DEFINES> <INCLUDES> <FLAGS> <SOURCE> <OBJECT>'
else
rcopts=''
fi
[ '${{ matrix.type }}' = 'Debug' ] && options+=' -DCMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG='
[ '${{ matrix.type }}' = 'Release' ] && options+=' -DCMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE='
cmake -B bld -G Ninja ${options} \
"-DCMAKE_C_FLAGS=${cflags}" \
"-DCMAKE_RC_COMPILE_OBJECT=${rcopts}" \
'-DCMAKE_BUILD_TYPE=${{ matrix.type }}' \
-DCMAKE_UNITY_BUILD=ON \
-DCURL_WERROR=ON \
-DBUILD_EXAMPLES=ON \
-DENABLE_WEBSOCKETS=ON \
-DCURL_BROTLI=ON \
${{ matrix.config }}

@vszakats
Copy link
Member

vszakats commented Oct 5, 2024

@jay To sort out if it's unity-related, perhaps the easiest is to duplicate the job and disable unity in one of them with -DCMAKE_UNITY_BUILD=OFF. (it doesn't look like one, but it may be worth a test.)

I've only tested UWP builds in CI. With MinGW, it can often help to peek into the headers. They are available here: https://downloads.sourceforge.net/project/mingw-w64/mingw-w64/mingw-w64-release/mingw-w64-v12.0.0.tar.bz2
Or online:
https://github.com/mirror/mingw-w64/blob/93f3505a758fe70e56678f00e753af3bc4f640bb/mingw-w64-headers/include/fileapi.h#L67-L88

The header only defines it for non-UWP. The MS SDK defines it for UWP, suggesting a
MinGW header bug, and perhaps also an implib bug. Depending on which, it may help to
declare it manually for MinGW + UWP until the headers are fixed. If it's also missing from
its implib, this feature will have to be disabled for MinGW + UWP.

Since this function requires Win10, some sort of mechanism seems to be necessary anyway
to disable it for incompatible targets. To fix all this in one, maybe GetFullPathNameW could
be converted to a dynamic call and check at runtime if it's available. This could make it work
for all compatible targets, regardless of dev env.

@jay jay force-pushed the multibyte_long_path branch 2 times, most recently from 38861a8 to f4eb1c5 Compare November 22, 2024 08:33
@jay
Copy link
Member Author

jay commented Nov 22, 2024

Since this function requires Win10, some sort of mechanism seems to be necessary anyway
to disable it for incompatible targets.

Thanks. It looks like mingw does not declare GetFullPathNameW for UWP targeting old versions of Windows even though it's in those versions. I added a custom declaration for UWP mingw < win 10 and that seems to have worked.

@jay jay added the feature-window A merge of this requires an open feature window label Nov 23, 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 curl#13522
@jay jay force-pushed the multibyte_long_path branch from f4eb1c5 to 18650ea Compare December 22, 2024 04:54
@jay jay closed this in 18650ea Dec 22, 2024
@jay jay merged commit 18650ea into curl:master Dec 22, 2024
197 checks passed
@jay jay deleted the multibyte_long_path branch December 22, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-window A merge of this requires an open feature window Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

5 participants