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.

Sign up for GitHub

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

Jump to bottom

Actually append "-" to --range #7837

Closed
wants to merge 4 commits into from
Closed

Actually append "-" to --range #7837

wants to merge 4 commits into from

Conversation

verhovsky
Copy link
Contributor

@verhovsky verhovsky commented Oct 10, 2021 •
edited
Loading

Because the block is missing the else, it always executes and overwrites the result of the if block before it. So curl has been warning users that it has fixed their incorrect --range it but not actually fixing it.

@verhovsky
Copy link
Contributor Author

verhovsky commented Oct 10, 2021 •
edited
Loading

This was broken back in 2005 in a93af43#diff-e0cf5b28d9b6b600f0af2bc78e8fd30ec675fd731a5da86f0c4283ffc0e40176L2363

(I noticed this working on curlconverter/curlconverter#303)

@bagder
Copy link
Member

bagder commented Oct 10, 2021

Can you please elaborate on the problem this fixes?

@verhovsky
Copy link
Contributor Author

verhovsky commented Oct 11, 2021 •
edited
Loading

The HTTP Range header https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range must be in this format (where <unit> is usually bytes):

Range: <unit>=<range-start>-
Range: <unit>=<range-start>-<range-end>
Range: <unit>=<range-start>-<range-end>, <range-start>-<range-end>
Range: <unit>=<range-start>-<range-end>, <range-start>-<range-end>, <range-start>-<range-end>
Range: <unit>=-<suffix-length>

i.e. it must have a - somewhere, either trailing Range: bytes=200- (to signify that you want everything starting at the 200th byte) or leading Range: bytes=-200 (to signify that you want everything up to the 200th byte). A Range without a -, like Range: bytes=200 is not a valid range. Back in 2005 the curl man page used to say that you could pass in --range 9500 to get everything after the 9500th byte, but this never worked because servers expect a -, so the man page was updated to say that you need to pass in --range 9500- and curl was changed to helpfully append a - if the user passed in just a number but then this was broken in 2007 because the else was removed, so the code block that used to not run if the user passed in just a number without a - now always runs, overwriting the result of the code that appends the -.

We can see that --range 9500 doesn't work on example.com, which implements this header. This is what I see with curl 7.64.1 on macOS:

$ curl -v --range 200 example.com
Warning: A specified range MUST include at least one dash (-). Appending one 
Warning: for you!
*   Trying 93.184.216.34...
* TCP_NODELAY set
* Connected to example.com (93.184.216.34) port 80 (#0)
> GET / HTTP/1.1
> Host: example.com
> Range: bytes=200
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Accept-Ranges: bytes
< Age: 578770
< Cache-Control: max-age=604800
< Content-Type: text/html; charset=UTF-8
< Date: Mon, 11 Oct 2021 09:50:04 GMT
< Etag: "3147526947+ident"
< Expires: Mon, 18 Oct 2021 09:50:04 GMT
< Last-Modified: Thu, 17 Oct 2019 07:18:26 GMT
< Server: ECS (sec/973B)
< Vary: Accept-Encoding
< X-Cache: HIT
< Content-Length: 1256
< 
<!doctype html>
<html>
<head>
[...]

As you can see, curl warned me that --range 200 is not a valid range, and that it (supposedly) appended - to make it as if I had passed --range 200-

Warning: A specified range MUST include at least one dash (-). Appending one 
Warning: for you!

but the actual Range: that got sent to the server is missing the -

> Range: bytes=200

and the server just ignores the (invalid) header and returns the whole page

< Content-Length: 1256
< 
<!doctype html>
<html>
[...]

This is because curl adds the - on this line

curl/src/tool_getparam.c

Line 1950 in 521bbbe

msnprintf(buffer, sizeof(buffer), "%" CURL_FORMAT_CURL_OFF_T "-", off);

curl/src/tool_getparam.c

Line 1952 in 521bbbe

config->range = strdup(buffer);

but then overwrites it with the argument exactly as it was passed in on this line

curl/src/tool_getparam.c

Line 1971 in 521bbbe

GetStr(&config->range, nextarg);

With the change in this PR, it actually appends the -:

Warning: A specified range MUST include at least one dash (-). Appending one 
Warning: for you!
*   Trying 93.184.216.34:80...
* Connected to example.com (93.184.216.34) port 80 (#0)
> GET / HTTP/1.1
> Host: example.com
> Range: bytes=200-
> User-Agent: curl/7.80.0-DEV
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 206 Partial Content
< Accept-Ranges: bytes
< Age: 282624
< Cache-Control: max-age=604800
< Content-Range: bytes 200-1255/1256
< Content-Type: text/html; charset=UTF-8
< Date: Mon, 11 Oct 2021 09:55:43 GMT
< Etag: "3147526947"
< Expires: Mon, 18 Oct 2021 09:55:43 GMT
< Last-Modified: Thu, 17 Oct 2019 07:18:26 GMT
< Server: ECS (sec/96CA)
< Vary: Accept-Encoding
< X-Cache: HIT
< Content-Length: 1056
< 
t="width=device-width, initial-scale=1" />
    <style type="text/css">
    body {
[...]

The header that gets sent is a valid range

> Range: bytes=200-

and the server responds with the file starting at the 200th byte:

< Content-Length: 1056
< 
t="width=device-width, initial-scale=1" />
    <style type="text/css">
    body {
[...]

So either curl should actually append the - to the header as the warning says it is doing (which is what this PR does), or (because evidently this doesn't seem to have bothered anyone enough to notice it for 14 years) the warning should be changed to not falsely say that it appends a - and the rest of the code in the if block can be removed.

@bagder
Copy link
Member

bagder commented Oct 11, 2021

Thanks for that, now I'm with you a 100%. I've made a test case locally that reproduces this problem and verifies your PR.

@bagder bagder closed this in 8758a26 Oct 11, 2021
@bagder
Copy link
Member

bagder commented Oct 11, 2021

Thanks!

@verhovsky verhovsky deleted the add-dash-to-range branch October 26, 2021 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
cmdline tool hacktoberfest-accepted
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@verhovsky @bagder @mback2k