Fix curl creating download dir in 750 mode

[moritz-h]

This suggests to fix the download directory being created with 750 mode on unix like systems.
Currently, the downloads directory is not readable by other users. This is inconvenient, e.g., for debugging CI systems, when the build directory is not fully readable. Further, this is an inconsistency to all other directories created by vcpkg (buildtrees, packages, installed, ...). All other directories are created with the default mkdir mode.

I traced the problem down to curl being used with the --create-dirs option to create all download directories. Curl always uses mode 750 to create directories as written in the docs.

This PR fixes the problem by creating the download directory in advance. I found an other usage of --create-dirs in the bulk download code: https://github.com/microsoft/vcpkg-tool/blob/2024-06-10/src/vcpkg/base/downloads.cpp#L481
But it seems file system access is not already available there, so maybe waiting for feedback if that solution is valid at all before trying to fix this there.

[dg0yt]

directory being created with 750 mode on unix like systems.

This seems surprising given that there is umask. That curl code is at least 13 years old. I wonder if it is intentional.

[moritz-h]

This seems surprising given that there is umask. That curl code is at least 13 years old. I wonder if it is intentional.

It seems intentional as it was documented resulting from this discussion: curl/curl#4766
In that discussion it was also mentioned that umask is respected. Only 750 is used as default before applying umask.

[dg0yt]

Hm, this discussion isn't entirely neutral...

The point isn't that umask isn't applied. The point is that curl starts with a more restrictive mode than expected (from common practice), and this is surprising. The world part of umask is rendered useless. Users must explicit call chmod to relax world restrictions.

(One might see this is as a security feature... but calling chmod is a security trap: No umask applied. Easy to make things world writable.)

[moritz-h]

a more restrictive mode than expected (from common practice), and this is surprising.

That is exactly the point for this PR. We probably have the same opinion on this.
I think its strange that the download folder has not default folder permission applied compared to a mkdir call on that system. The curl discussion likely was not optimal especially from the request side. However, it was made clear from curl that 750 is the intended behaviour. Therefore, it may be fixed in vcpkg.
I also do not like to play around with file permissions after the curl call, especially as you may need to track what part of the path existed before and which folders are newly created. Thats why I'm suggesting to just create the folders in advance here.

[BillyONeal]

At a minimum this needs a test and to remove the --create-dirs part. The purpose of --create-dirs is to deal with racy behavior: The right behavior isn't to try to create directories before doing a thing, it's to try to create the file directly, and only if that fails fall back to trying to create directories and the hope is that curl is able to do such behavior.

[BillyONeal]

I'm not sure this is the right thing, there are a lot of edge cases where just chopping off the last path element will do the wrong thing. Consider something like a/b/../out.exe.

[moritz-h]

The purpose of --create-dirs is to deal with racy behavior: The right behavior isn't to try to create directories before doing a thing, it's to try to create the file directly, and only if that fails fall back to trying to create directories and the hope is that curl is able to do such behavior.

My thought was, if the directory does not exist I know in advance that curl will not create it and fail without passing --create-dirs. If the directory already exists create_directories is a no-op. The same pattern was used above in download_winhttp: https://github.com/microsoft/vcpkg-tool/blob/2024-06-10/src/vcpkg/base/downloads.cpp#L715-L717
The race condition I see is someone else is deleting the directory between creation and curl execution, that is why I initially kept --create-dirs as an extra fallback layer, so in that case the bad effect would be "only" having a directory with wrong permission and not fail the curl call. But yes probably makes sense to simplify this to have only one case.
An other drawback would be in case the download fails, I still potentially created an empty directory as side effect.

I still think this is maybe the better option compared to trying to fix directory permissions after the curl call, but if there are any better ideas I'm open to try them.
So should I try to continue to polish this, i.e., by adding canonical path check for the /../ situation, do you have other ideas how this should be solved or do you similar to curl not see this as a problem at all and I need to fix it in my code which calls vcpkg?

Side note:
I found a more recent request in the curl repo for this feature, so I think this will definitely not be fixed in curl:
curl/curl#6757 and curl/curl#6759