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

z_vcpkg_download_distfile_via_aria() uses malformed arguments to aria2 download command #30079

Open
craigscott-crascit opened this issue Mar 8, 2023 · 9 comments
Assignees
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.

Comments

@craigscott-crascit
Copy link

craigscott-crascit commented Mar 8, 2023

Describe the bug

In the implementation of z_vcpkg_download_distfile_via_aria(), the following call (from here) is made to use aria2 to download a file:

        vcpkg_execute_in_download_mode(
            COMMAND ${ARIA2} ${URL}
            -o temp/${arg_FILENAME}
            -l download-${arg_FILENAME}-detailed.log
            ${headers_param}
            OUTPUT_FILE download-${arg_FILENAME}-out.log
            ERROR_FILE download-${arg_FILENAME}-err.log
            RESULT_VARIABLE error_code
            WORKING_DIRECTORY "${DOWNLOADS}"
        )

The -o temp/${arg_FILENAME} argument is malformed. That must not contain any path, it must only be a file name. The -d <dir> option must be used to specify the path instead.

When a path is included in the -o option, it breaks downloads where a redirection is involved. The original URL is first downloaded to the file name specified with the -o option, which is in the temp subdirectory. aria2 checks the HTTP response code and contents of that file and determines that it is a redirect and it needs to try the location specified in the returned content. When it does this retry, the download-${arg_FILENAME}-detailed.log file shows that the temp part of the output file name has been dropped, and now it is downloading the retry to the working directory instead. This succeeds and the aria2 command returns success. However, z_vcpkg_download_distfile_via_aria() then ends up calling z_vcpkg_download_distfile_test_hash() a bit further below and it tests the file in the temp subdirectory, not the final downloaded file in the working directory:

        z_vcpkg_download_distfile_test_hash(
            "${DOWNLOADS}/temp/${arg_FILENAME}"
            "downloaded file"
            "The file may have been corrupted in transit."
            "${arg_SHA512}"
            ${arg_SKIP_SHA512}
        )

This test fails because that file contains content about the redirection, not the final downloaded file. This results in a fatal error, but importantly it also leaves behind the requested downloaded file in the working directory. If you then re-run exactly the same command a second time, it will succeed because the download gets skipped now that the file is there and has the expected hash.

As an example where this bites users, the openssl port for versions before 3.0.7-1 require the jom tool on Windows (#27150 added a fallback to using nmake if using jom results in an error). It calls vcpkg_find_acquire_program(JOM), which eventually finds its way to calling vcpkg_download_distfile(), which then calls z_vcpkg_download_distfile_via_aria(). In vcpkg_find_acquire_program(), the first URL it provides for JOM is https://download.qt.io/official_releases/jom/jom_1_1_3.zip which returns a redirection, triggering the bug.

Environment

  • Windows 64-bit
  • Compiler: MSVC, but I doubt it matters

To Reproduce

  1. Checkout the 2021.08.31 tag of vcpkg. This is just to make the steps easy, the latest master still has the bug. Latest master will also show the bug if you start with a clean vcpkg repo (more critically, you don't have a jom executable in downloads/tools/jom/jom-1.1.3).
  2. Make sure downloads/jom_1_1_3.zip does not exist. Delete it if you have that file from some earlier runs. This is critical!
  3. ./vcpkg install openssl:x64-windows

The above will try to install openssl 1.1.1m (or a later version if you're using vcpkg master), but will fail. The error claims that the downloaded jom_1_1_3.zip file has a different hash to what was expected.

If you now re-run exactly the same command, it will succeed on this second attempt.

Expected behavior

The jom tool should be downloaded successfully on the first try.

Failure logs

Sorry, can't attach logs for privacy reasons in this case.

@JonLiu1993 JonLiu1993 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Mar 8, 2023
@Cheney-W Cheney-W self-assigned this Mar 8, 2023
@Cheney-W
Copy link
Contributor

Cheney-W commented Mar 8, 2023

Thanks for reporting this issue to vcpkg!

Unfortunately, I can't reproduce this issue in my side, I have tested the following two scenarios:

  1. Git clone a new vcpkg and then checkout to the 2021.08.31 tag, the jom_1_1_3.zip could be downloaded successfully.
  2. Git clone a new vcpkg and use manifest mode to install openssl 1.1.1m#2, the jom_1_1_3.zip could be downloaded successfully.

I have checked the vcpkg_download_distfile.cmake file and I found that when installing the openssl, because _VCPKG_DOWNLOAD_TOOL has no value, z_vcpkg_download_distfile_via_aria() is not executed. This should be the reason why I can't reproduce the issue, so I want to confirm with you whether there are any operations on aria2 to reproduce this issue?

@craigscott-crascit
Copy link
Author

Apologies for the delay in replying. Make sure you don't already have anything in your source tree from earlier work. It is critical that you don't have anything downloaded from earlier steps or else it will mask the underlying problem.

Make sure you follow the exact steps I outlined. The sequence is very specific. Unfortunately, it is also what a typical CI job is going to do if it is using aria for its downloads. I don't know why it is using aria in my case (both locally and in CI), I don't appear to have any thing set to force that.

I suggest trying to do this from different shells. I think if you use Powershell, you get burnt by a dodgey pseudo curl command (lost days chasing that a year ago when exploring curl v aria2 for downloads). Try a standard Dos/Command prompt instead. That would more closely match what I have been doing locally too.

Even if you can't reproduce it though, note that the issue description details how the aria code path is doing the wrong thing. I assume you've got some testing somewhere that exercises that code path. You just need to point it at something that comes back with a redirect to replicate the problem.

@Cheney-W
Copy link
Contributor

Thanks for your reply. I will try again.

@craigscott-crascit
Copy link
Author

You could also potentially try setting the VCPKG_INSTALL_OPTIONS CMake variable to --x-use-aria2. I see that is being done in our own CI jobs, but I didn't have to do that when testing locally.

@github-actions github-actions bot added the Stale label Oct 10, 2023
@microsoft microsoft deleted a comment from github-actions bot Oct 10, 2023
@Cheney-W Cheney-W removed the Stale label Oct 10, 2023
Copy link

github-actions bot commented Apr 9, 2024

This is an automated message. Per our repo policy, stale issues get closed if there has been no activity in the past 180 days. The issue will be automatically closed in 14 days. If you wish to keep this issue open, please add a new comment.

@github-actions github-actions bot added the Stale label Apr 9, 2024
@Cheney-W
Copy link
Contributor

Stay active

@github-actions github-actions bot removed the Stale label Apr 16, 2024
@senstar-nross
Copy link

@Cheney-W I just repro'd a very similar error building opencv4 with --x-use-aria2

And after fixing the debug_message on line 70 of vcpkg_download_distfile.cmake:

ie: filename should be arg_FILENAME

debug_message("Download Command: ${ARIA2} ${URL} -o temp/${filename} -l download-${filename}-detailed.log ${headers_param}") 

This is the error I get. Notice the output filename has subdirectories which fails to download

           -- Using source at C:/vcpkg/buildtrees/opencv4/src/4.8.0-f4e8005717.clean
           -- Downloading opencv-cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz...
           -- [DEBUG] Download Command: C:/vcpkg/downloads/tools/aria2-1.37.0-windows/aria2-1.37.0-win-64bit-build1/aria2c.exe https://github.com/tiny-dnn/tiny-dnn/archive/v1.0.0a3.tar.gz -o temp/opencv- 
         cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz -l download-opencv-cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz-detailed.log
           -- Downloading opencv-cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz... Failed.
               Exit Code: 1
               See logs for more information:
                   C:/vcpkg/downloads/download-opencv-cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz-out.log
                   C:/vcpkg/downloads/download-opencv-cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz-err.log
                   C:/vcpkg/downloads/download-opencv-cache/tiny_dnn/adb1c512e09ca2c7a6faef36f9c53e59-v1.0.0a3.tar.gz-detailed.log

@BillyONeal
Copy link
Member

BillyONeal commented Jan 15, 2025

Given that aria2 has apparently been broken since at least May 2023, we do not test for it, only 2 people have noticed, large swaths of the tool like fetch do not listen to the option, it creates circularity issues because we need some other downloader to get aria2 in the first place, etc. I think we should remove --x-use-aria2 entirely.

@BillyONeal BillyONeal added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 15, 2025
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this issue Jan 22, 2025
* Huge parts of the vcpkg product do not respect this setting
* microsoft/vcpkg#30079 shows it has been entirely broken for an extremely long time.
* It's an --x switch and we never claimed full support for it.
* We have never tested that it ever actually works.

Related: microsoft/vcpkg-docs#441
BillyONeal added a commit to BillyONeal/vcpkg that referenced this issue Jan 23, 2025
@BillyONeal
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

No branches or pull requests

6 participants
@BillyONeal @craigscott-crascit @Cheney-W @JonLiu1993 @senstar-nross and others