r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

Enable long path awareness.

A minor concern I had is that there is a fairly significant amount of C code that cargo uses. What are the chances this would cause a problem (like a buffer overflow) with that? It seems pretty unlikely to me, but I don't know which APIs might be at risk. Is it just looking out for usage of MAX_PATH?

Yes.

I think the risk is low. If C code does use fixed sized MAX_PATH buffers, then it should still be sending the right array bounds to the Windows API or checking a user provided path fits in the buffer. That said, buffer overflows are a common issue in general so I don't want to discount the possibility entirely. Though it helps that long path awareness has been a thing for years at this point.

Btw, I'm not sure why check-version-bump failed. It doesn't seem related to this PR? But maybe I'm missing something.

error: failed to run custom build command for `curl-sys v0.4.69 curl-8.5.0`

Caused by:
  process didn't exit successfully: `/home/runner/work/cargo/cargo/target/semver-checks/local-crates_io-0_39_2/target/semver-checks/target/debug/build/curl-sys-070b7960e8af9c11/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-changed=curl
  cargo:rerun-if-env-changed=LIBCURL_NO_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG
  cargo:rerun-if-env-changed=LIBCURL_STATIC
  cargo:rerun-if-env-changed=LIBCURL_DYNAMIC
  cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
  cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
  Couldn't find libcurl from pkgconfig (`PKG_CONFIG_ALLOW_SYSTEM_CFLAGS="1" PKG_CONFIG_ALLOW_SYSTEM_LIBS="1" "pkg-config" "--libs" "--cflags" "libcurl"` did not exit successfully: exit status: 1
  error: could not find system library 'libcurl' required by the 'curl-sys' crate

  --- stderr
  Package libcurl was not found in the pkg-config search path.
  Perhaps you should add the directory containing `libcurl.pc'
  to the PKG_CONFIG_PATH environment variable
  No package 'libcurl' found
  ), compiling it from source...
  cargo:root=/home/runner/work/cargo/cargo/target/semver-checks/local-crates_io-0_39_2/target/semver-checks/target/debug/build/curl-sys-5c163e2bf80bc632/out
  cargo:include=/home/runner/work/cargo/cargo/target/semver-checks/local-crates_io-0_39_2/target/semver-checks/target/debug/build/curl-sys-5c163e2bf80bc632/out/include
  cargo:static=1
  cargo:rustc-cfg=libcurl_vendored

  --- stderr
  fatal: not a git repository (or any of the parent directories): .git
  thread 'main' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/curl-sys-0.4.69 curl-8.5.0/build.rs:86:10:
  called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

error: process didn't exit successfully: `cargo semver-checks check-release --workspace` (exit status: 1)

Probably due to alexcrichton/curl-rust#542.

OK, I did a little bit of auditing. I think these should mostly be OK, but I'm always a little worried about what I don't know here.

home uses a fixed-width MAX_PATH buffer, but SHGetFolderPathW is documented to not return anything longer. I don't know if Windows just returns an error if it is too long? Or maybe it doesn't allow special folders like a home directory to be defined to be too long?

is-terminal uses a fixed width buffer, but passes the length into GetFileInformationByHandleEx. (Wish we could drop that dependency.)

sqlite is limited to MAX_PATH in a lot of places, but I'm going to assume all its usage is correct. It can be compiled with a define to use a longer path, but I don't think that is immediately necessary. There is a special VFS to support 32K paths, but that requires special handling. We'll need to consider this if we start using sqlite in the target directory, and we want to support target directories that exceed MAX_PATH.

curl has a disturbingly large amount of MAX_PATH routines that I don't feel inclined to review. Hopefully someone out there has already done that. 🤷

git2 also has a bunch of MAX_PATH buffers, but looks to usually validate lengths. However, I also don't feel inclined to review. It is a little worrisome that it appears to support paths longer than MAX_PATH even though there are code paths that seem to have MAX_PATH buffers. I'm not sure how it does that. It seems to honor core.longpaths which defaults to false. I'm not sure if we should consider setting that ourselves?

There's a few other little things (like openssl), but none jumped out to me.

It looks like this might help a few long-path scenarios, but doesn't cover everything. IIUC, link.exe does not handle paths longer than MAX_PATH. The current working directory cannot be longer than MAX_PATH (which is an issue for running cargo itself). AFAIK, Rust does not allow launching processes with paths longer than MAX_PATH (which is an issue for build scripts and such). Does that sound roughly correct?

I think home should be using SHGetKnownFolderPath but it shouldn't matter too much either way. A lot of things would break if the home folder were longer than MAX_PATH.

is_terminal is fine. It's looking for a (roughly) known pipe path. If the path is longer then that's just an error and the function assumes it won't match the path being looked for. (btw, can't you replace it with the std IsTerminal trait?)

Looking through a few of the curl/sqlite/git2 hits, I'm not too worried by what I've seen so far. I think the main issue would be UB due to incorrect assumptions which I'm not seeing. I'm not so worried about cases where the buffer is simply too small. That's an error but it's no worse than the current situation and can be fixed.

libgit2 is however funny in that it has to artificially enforce the MAX_PATH limit if core.longpaths isn't set. And it always enforces MAX_PATH for the .git directory just in case someone disables the option. I assume this limitation will exist so long as the option does.

link.exe does not handle paths longer than MAX_PATH.

True. Though this could change.

The current working directory cannot be longer than MAX_PATH (which is an issue for running cargo itself).

With longpathaware it can be.

AFAIK, Rust does not allow launching processes with paths longer than MAX_PATH (which is an issue for build scripts and such).

It should do now.

Oh, wait the combination of both running a process and setting the current directory might not work together (i.e. Command::current_dir). I'd need to test that. std::env::set_current_dir should work though.

EDIT: Yeah, launching a process with a current directory longer than MAX_PATH doesn't work.

is_terminal is fine. It's looking for a (roughly) known pipe path. If the path is longer then that's just an error and the function assumes it won't match the path being looked for. (btw, can't you replace it with the std IsTerminal trait?)

supports-hyperlinks is small enough that I'm willing to drop it if they aren't willing to bump MSRV. let me know if you want me to explore this.

The current working directory cannot be longer than MAX_PATH (which is an issue for running cargo itself).

With longpathaware it can be.

I'm unable to get this to work. Perhaps it depends on the shell? I have Windows 10 22H2 (19045.3693), with the LongPathsEnabled registry entry set, running in Windows Terminal, using cargo built with this PR. Running cargo prints:

• cmd prints The current directory is invalid.
• git bash prints Error: Current working directory has a path longer than allowed for a Win32 working directory. Can't start native Windows application from here.
• Powershell 7.4 prints nothing. There is a window that opens and closes instantly, too fast to see. Nothing else happens.
• Powershell 5.1 prints cargo.exe failed to run: The directory name is invalid with a bunch more information.

Rust does not allow launching processes with paths longer than MAX_PATH (which is an issue for build scripts and such).

Ah, that does seem to work now if there is a manifest. However, for processes that don't have a manifest (like build scripts launching other processes), it fails. I get Command::status() returns NotFound.

See my next comment. Using std::env::set_current_dir in a longpathaware application works. But launching a new process with a long current directory does not.

I've created an issue about home in case that's something people want to consider. I don't feel particularly strongly either way but I guess I have a general preference towards not using deprecated functions even if they stick around forever.

Ok, so I think that having long path awareness is on the whole better than not having it. It can help in a few cases and, from what I've seen, the C dependencies seem fine with it. I admittedly haven't fully audited it all so I can't be certain but given that long path awareness has been around since ~2016, it's not like we're blazing a trail here. Still, I can understand if Cargo wants to be more cautious. I'd be happy to update this PR to remove the longPathAware option and consider that question separately if that would be preferable.

The issue of setting the current directory is more concerning. Cargo uses set_current_dir in one place, when the -C option is used to override the current directory. Maybe cargo could warn when the current directory is set to a path greater than MAX_PATH?

I'll go ahead and approve. I think on balance, it probably should be safe, and does help with a few, although limited circumstances.

In the future we may want to consider moving this file (and possibly build.rs?) to a separate directory. I'm not sure how we will want to organize stuff like that, since I can really predict what we might need in the future. But if someone on the cargo team wants to do that sooner, that sounds fine to me, too.

Regarding set_current_dir, I'll follow up with filing some more issues.

@bors r

📌 Commit 6f7f927 has been approved by ehuss

It is now in the queue for this repository.

⌛ Testing commit 6f7f927 with merge 232f913...

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 232f913 to master...