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

ftp: fix for FTP growing file support #9772

Closed

Conversation

fractal-access
Copy link
Contributor

When using the option CURLOPT_IGNORE_CONTENT_LENGTH (set.ignorecl in code) to support growing files in FTP, the code should ignore the initial size it gets from the server as this will not be the final size of the file. This is done in ftp_state_quote() to prevent a size request being issued in the initial sequence. However, in a later call to ftp_state_get_resp() the code attempts to get the size of the content again if it doesn't already have it, by parsing the response from the RETR request. This fix prevents this parsing of the response to get the size when the set.ignorecl option is set. This should maintain the size value as -1, unknown, in this situation.

When using the option CURLOPT_IGNORE_CONTENT_LENGTH (set.ignorecl in code) to support growing files in FTP, the code should ignore the initial size it gets from the server as this will not be the final size of the file. This is done in ftp_state_quote() to prevent a size request being issued in the initial sequence. However, in a later call to ftp_state_get_resp() the code attempts to get the size of the content again if it doesn't already have it, by parsing the response from the RETR request. This fix prevents this parsing of the response to get the size when the set.ignorecl option is set. This should maintain the size value as -1, unknown, in this situation.
@fractal-access
Copy link
Contributor Author

Testing

To find this issue and test I have done the following. I created some code to slow copy a file at 5 KB/s. I then use a standard curl_easy_perform() to get this file from an FTP server while it grows. This will cause libcurl to retrieve the amount of file based on the size it gets at the start of the interaction and raise and error due to getting a, 450 Transfer aborted, error from the server. I then set the option:

curl_easy_setopt(m_curlHandle, CURLOPT_IGNORE_CONTENT_LENGTH, 1L)

I would expect this to now get the complete file, but it fails in the same way as before. After applying the fix, libcurl continues reading the data connection until the server indicates the end. I then get the full file after it has finished being slow copied.

For my tests I used the ProFTPD server with a tailmode filter addition to support growing files. The main function of this is to not automatically finish the transfer if the server reaches eof, but to also make sure the file hasn't been updated within a timeout period. I think this could be tested against a standard FTP server if the bandwidth option is also used in libcurl and set to a value smaller than the growing file rate.

I not sure how easy it would be to get this type of test into the libcurl test suite, but if anyone has any thoughts on this please let me know.

@dfandrich
Copy link
Collaborator

dfandrich commented Oct 20, 2022 via email

@bagder bagder added the FTP label Oct 20, 2022
@fractal-access
Copy link
Contributor Author

I not sure how easy it would be to get this type of test into the libcurl test suite, but if anyone has any thoughts on this please let me know.
How about returning a small size in response to the SIZE command, but actually return a much longer file, as though it were added to during the transfer? test135 is probably a good one to use as a template for this.

That's a good idea, thanks for the suggestion.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test.

Added setting: RETRSIZE [size] in the <servercmd> section. When set this will cause the test FTP server to return the size set (rather than the actual size) in the acknowledgement from a RETR request.
@fractal-access
Copy link
Contributor Author

I have attempted to make a test which hopefully is acceptable, but please check.

I added a new test at the first available number, 416, and based it off test 135.

After looking into this I found to test the real situation I needed to get the test FTP server to reply with a smaller size than the final size of the data in the RETR command.
I wasn't sure of the best way to do this. I've gone for adding another value in the section. I have added RETRSIZE [size], that when set will explicitly set the value of the size returned by ftpserver.pl. In ftpserver.pl, I have tried to follow the scheme for other value settings.

@bagder
Copy link
Member

bagder commented Nov 14, 2022

The REPLY command already exists to provide a custom response for a given FTP command. No need to invent another command for the same thing!

@fractal-access
Copy link
Contributor Author

Thanks for the suggestion. As far as I can tell REPLY will fake the response but doesn't actually perform the request. The test needs the fake size and also the data to be sent. Is there a configuration using the current parameters that allows for the fake response as well as actually getting the request performed?

@bagder
Copy link
Member

bagder commented Nov 18, 2022

I figured you just want to provide a regular request/transfer and send X bytes in a response but have the SIZE response say the data is Y bytes, where Y is smaller than X. The way a transfer of a growing file would appear.

@fractal-access
Copy link
Contributor Author

The implementation of --ignore-content-length/CURLOPT_IGNORE_CONTENT_LENGTH is that it skips the SIZE command in the standard libcurl FTP interaction. Because of this it ends up getting the size from the parse of the return from the RETR command. Therefore, the test needs to send a smaller length value back to the RETR request to mimic the interaction.

@bagder
Copy link
Member

bagder commented Nov 18, 2022

Yes, which is why you make the RETR response say size X while the response returns size Y.

@fractal-access
Copy link
Contributor Author

Yes, that's correct. Can you think of a way to do this without having to introduce a new parameter?

@bagder
Copy link
Member

bagder commented Nov 24, 2022

Looking into this made me question the need for this PR. The functionality already exists and seems to work!

Test 1137 already tests FTP with --ignore-content-length. How is that not enough?

@fractal-access
Copy link
Contributor Author

Growing files don't currently work on the latest libcurl version. This can been seen by looking at the code, where it can be seen that the size will still be set by parsing the return from the RETR command. It can also be shown by running the new test against the current version, which will fail.

Test 1137 uses --ignore-content-length, but does not test growing files.

@bagder
Copy link
Member

bagder commented Nov 24, 2022

I disagree.

The current code already works with growing files if you use --ignore-content-length.

It does this by avoiding the SIZE command all together and thus it does not know how large the file is and it only uses the disconnect as a signal to when the file ends.

Test 1137 verifies this. You can see that it does not send a SIZE, and when it doesn't know the size upfront, it also does not know if the transferred files grows or shrinks or whatever. curl just accepts what it gets and considers the file complete when the connection is closed.

If you look at what curl does when your test 416 is run, you will see that your extra work for the ftp server is not even used.

@fractal-access
Copy link
Contributor Author

There has never been any doubt that --ignore-content-length turns off the SIZE request. The issue is that curl will also establish the size by parsing the length out of the RETR request return. Do you think this is not the case? If so, can you explain what the code does from line 2457 of ftp.c?

This can also be seen with a simple test with verbose turned on. The size curl has is printed at 2515:

infof(data, "Getting file with size: %" CURL_FORMAT_CURL_OFF_T,
size);

If --ignore-content-length worked this should print out -1, agreed?

Try something like:

curl ftp://hlm:hlm@maudv03//highres/nd/destination.mxf --ignore-content-length -v

which gives:

Getting file with size: 1024000
Not
Getting file with size: -1

@bagder
Copy link
Member

bagder commented Nov 26, 2022

Ah yes, I forgot about that weirdness.

Edit: which of course is the entire point with your PR.

bagder
bagder approved these changes Nov 26, 2022
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I've slowly recalled how this works. I think this is the way to do it.

@bagder bagder closed this in 76b3f5f Nov 26, 2022
bagder pushed a commit that referenced this pull request Nov 26, 2022
Added setting: RETRSIZE [size] in the <servercmd> section. When set this
will cause the test FTP server to return the size set (rather than the
actual size) in the acknowledgement from a RETR request.

Closes #9772
@bagder
Copy link
Member

bagder commented Nov 26, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants