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

Add CURLOPT_TFTP_NO_OPTIONS for TFTP servers that ignore the option exension #481

Closed
bagder opened this Issue Oct 8, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@bagder
Member

bagder commented Oct 8, 2015

Some TFTP server implementations ignore the "TFTP Option extension" (RFC 1782-1784, 2347-2349), or implement it in a flawed way, causing problems with libcurl. Another switch for curl_easy_setopt "CURLOPT_TFTP_NO_OPTIONS" is introduced which prevents libcurl from sending TFTP option requests to a server, avoiding many problems caused by faulty implementations.

patch by Michael König

@jay

This comment has been minimized.

Member

jay commented Nov 2, 2015

@bagder I'd like to see this feature get in if possible, I didn't realize it didn't make it in last round. I already took a look back then and suggested some changes at https://github.com/jay/curl/compare/master...jay:tftp_no_options?expand=1. You said you were going to help with a test case and I had one reservation regarding blocksize:

Now that we're not sending options (tsize, blksize, timeout), someone will need to check that there is not any code that depends on any of those being sent. As far as I can tell no but someone experienced in this protocol may want to take a look. The blocksize for example appears will default to 512 so even if a user specifies a bigger blocksize and the server doesn't return a blocksize 512 will be used.

That's probably fine, but I'm not 100%.

@mkoenig75

This comment has been minimized.

mkoenig75 commented Nov 5, 2015

@jay About your reservation:
To my knowledge there is no back channel for option information. So even if the application was using the option to change the BLKSIZE it would never know if the server agreed to it or not.

The only way is through educated guessing in the data callbacks. And since the server was never required to agree to anything i see no problem here. You always needed error handling in case the BLKSIZE was not what you expected.

If you meant code inside libcurl, i did not encounter anything myself.

@bagder

This comment has been minimized.

Member

bagder commented Nov 6, 2015

I just pushed commit 60c8629, which makes all TFTP test cases verify that the exact set of options is passed to the server so checking for a lack of those options is now also possible.

@jay jay self-assigned this Nov 6, 2015

@jay

This comment has been minimized.

Member

jay commented Jan 7, 2016

@bagder Where are we with the feature window? I can't find a notice it will be closed. I have some time tomorrow and I'd like to land this, it just needs a test. Also I'd like to get in CURLINFO_TLS_SSL_PTR, I relent on my bikeshedding over the name.

@bagder

This comment has been minimized.

Member

bagder commented Jan 7, 2016

ouch, I completely forgot to post a note about it. But as you probably felt, we're beyond that now for 7.47.0 - release date is set for Jan 27th. (I'll send an email now)

jay added a commit that referenced this issue Feb 23, 2016

TFTP: add option to suppress TFTP option requests (Part 1)
Some TFTP server implementations ignore the "TFTP Option extension"
(RFC 1782-1784, 2347-2349), or implement it in a flawed way, causing
problems with libcurl. Another switch for curl_easy_setopt
"CURLOPT_TFTP_NO_OPTIONS" is introduced which prevents libcurl from
sending TFTP option requests to a server, avoiding many problems caused
by faulty implementations.

Bug: #481

jay added a commit that referenced this issue Feb 23, 2016

TFTP: add option to suppress TFTP option requests (Part 2)
- Add tests.

- Add an example to CURLOPT_TFTP_NO_OPTIONS.3.

- Add --tftp-no-options to expose CURLOPT_TFTP_NO_OPTIONS.

Bug: #481
@jay

This comment has been minimized.

Member

jay commented Feb 23, 2016

@mkoenig75 Landed in 9dc3eae and 186546f. Thanks and sorry for the long delay.

@jay jay closed this Feb 23, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.