Get-FtpFile fails with integer overflow when downloading file more than 2gb in size

[jamestelfer]

What You Are Seeing?

When downloading a >2Gb file via ftp with Get-FtpFile (called by Get-ChocolateyWebFile), the following error is observed:

Running Get-FtpFile -url 'ftp://<elided>/files/iso/Microsoft/SQL/2016/en_sql_server_2016_developer_with_service_pack_1_x64_dvd_9548071.
iso' -fileName 'C:\ProgramData\chocolatey\lib\sql-server-developer\tools\local.iso'
ERROR: The remote file either doesn't exist, is unauthorized, or is forbidden for url 'ftp://<elided>/files/iso/Microsoft/SQL/2016/en_s
ql_server_2016_developer_with_service_pack_1_x64_dvd_9548071.iso'. Cannot convert value "2147483648" to type "System.Int32". Error: "Value was eit
her too large or too small for an Int32."
 at Get-FtpFile, C:\ProgramData\chocolatey\helpers\functions\Get-FtpFile.ps1: line 196
at Get-ChocolateyWebFile, C:\ProgramData\chocolatey\helpers\functions\Get-ChocolateyWebFile.ps1: line 345
at <ScriptBlock>, C:\ProgramData\chocolatey\lib\sql-server-developer\tools\chocolateyInstall.ps1: line 50
at <ScriptBlock>, C:\ProgramData\chocolatey\helpers\chocolateyScriptRunner.ps1: line 48
at <ScriptBlock>, <No file>: line 1

What is Expected?

The file should download successfully.

How Did You Get This To Happen? (Steps to Reproduce)

1. Host a large (>2Gb) file on an FTP server to which you have access
2. Download file using the Get-FtpFile function

Output Log

https://gist.github.com/jamestelfer/f10314dab426b9e47c447ca3f9db76bd

[jamestelfer]

Note also in the log that the local destination file remained locked after the FTP download failed, hindering choco's cleanup.

[ferventcoder]

That is definitely an issue. Marked for fixing soon.

[ferventcoder]

Also marking this as up for grabs if you are interested

[jamestelfer]

I've forked the repo, let me have a quick squiz. Looking at the Get-WebFile source, it shouldn't be difficult for the actual fix (for this and #1099). Not sure about the testing strategy for this one though aside from manual checks.

[jamestelfer]

I'm happy to work through with you on this one, though I'd appreciate some guidance on how you think it's best tested.

Looking at the code (also thinking of #1099) there is some divergence from Get-WebFile in three main areas:

1. The variable type of the $goal, $total and $count variables
2. Proxy handling is more advanced for Get-WebFile, though this may be HTTP-specific
3. The buffer size (Increase download buffer size in Get-FtpFile to speed up downloads #1099)
4. Progress output (once every 10 iterations for Get-WebFile)

In this issue, I'm concentrating on the first of these only to keep focused.

There are two other aspects of these functions:

1. The catch/throw/finally, where the catch just rethrows the exception. Is the catch block required for some supported versions of PS? Otherwise it could probably be removed.
2. In order to avoid locking the output file on error, the cleanup of the output streams should be in the finally clause IMO. Thoughts? This is the reason that the log output includes a file locked message.

I'll open some pull requests and look for your comments.

JT

[ferventcoder]

Finally block sounds good. The code in get-ftpfile cane in as a PR copying get-webfile and chamging it to ftp specific I believe.

[jamestelfer]

OK so I think I've finished messing around with effectively no-op commits on this issue to comply with the CLA correctly. I'll take a look at the file locking issues here.

Any reason to have the catch block in there that re-throws the exception? PowerShell doesn't rethrow that nicely, so it would be nice to avoid it. Is this a PowerShell version 1 idiom? What PS versions should I test on?

[ferventcoder]

PowerShell v2+

[ferventcoder]

Get-WebFile was brought in from an external location, so Get-FtpFile was based on that. Some of the code it has may not be the best.

[ferventcoder]

Any reason to have the catch block in there that re-throws the exception? PowerShell doesn't rethrow that nicely, so it would be nice to avoid it.

Can you point this specific bit out so I know we are talking about the same thing?

[jamestelfer]

Sorry mate, this bit.

      Write-Host ""
      Write-Host "Download of $([System.IO.Path]::GetFileName($fileName)) ($goalFormatted) completed."
    } catch {
      throw $_.Exception
    } finally {
        $ErrorActionPreference = $originalEAP
    }

    $reader.Close()

I suggest removing catch { throw $_.Exception } as the catch block is optional, and the error will naturally bubble. According to a code search, it appears that Choco doesn't assume anything about the ErrorActionPreference setting, so saving and restoring it appears valid.

[ferventcoder]

There may be something with PowerShell v2 (or really .NET Framework 2.0) where the catch block is not optional (hard to recall offhand though). The big part of that is I just would rather catch and throw (not rethrow), but maybe at the time could not determine how to do that with PowerShell. The real purpose was to ensure that the ErrorActionPreference was reset, so the finally block is the most important part of that. 👍

[jamestelfer]

I updated the resource handling on errors, and tested it on PS 2.0 and PS 5.1. I tested it by adding a deliberate throw in the read loop, then attempt to clean up the downloaded file fragment on failure. The call failed, and the file handle was not kept open allowing cleanup to occur.

I tried to improve the error handling, but it appears that PS doesn't have great support for chaining errors, so I gave that up.

Let me know if there are further modifications I should make.

[ferventcoder]

This will be included in 0.10.4