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

curl tool globbing range [1-1] doesn't work #1238

Closed
rdsteed opened this issue Feb 1, 2017 · 13 comments
Closed

curl tool globbing range [1-1] doesn't work #1238

rdsteed opened this issue Feb 1, 2017 · 13 comments
Assignees

Comments

@rdsteed
Copy link

rdsteed commented Feb 1, 2017

I recently upgraded curl.

My old version allowed allowed the curl command to use single value ranges eg [1-1]. In the new version this causes an error.

Is there any chance that curl could revert to the older, more permissive behavior?

@jay
Copy link
Member

jay commented Feb 1, 2017

Please fill out the template, it is not filled out

@rdsteed
Copy link
Author

rdsteed commented Feb 1, 2017

Sorry about the initial post missing information - snafu with github editor.

Addendum - current version information:
curl 7.52.1 (i386-pc-win32) libcurl/7.52.1 OpenSSL/1.0.2k zlib/1.2.8 nghttp2/1.19.0

Old version is unknown - lost in hard disk crash. Downloaded latest curl as part of recovery.

@jay
Copy link
Member

jay commented Feb 1, 2017

[1-1] was supported starting in b169aa2 but it likely broke in 5ca96cb which has glob fail when this is true: (step_n > (max_n - min_n)) or (1 > (1 - 1)). There is similar logic to stop something like [a-a]. I think that could be supported again, unless it was done intentionally. @bagder?

diff --git a/src/tool_urlglob.c b/src/tool_urlglob.c
index 0edfac6..b0e46b2 100644
--- a/src/tool_urlglob.c
+++ b/src/tool_urlglob.c
@@ -220,8 +220,9 @@ static CURLcode glob_range(URLGlob *glob, char **patternp,
 
     *posp += (pattern - *patternp);
 
-    if((rc != 3) || (min_c >= max_c) || ((max_c - min_c) > ('z' - 'a')) ||
-       (step <= 0) )
+    if(rc != 3 || step <= 0 ||
+       (min_c == max_c && step != 1) ||
+       (min_c != max_c && (min_c > max_c || (max_c - min_c) > ('z' - 'a'))))
       /* the pattern is not well-formed */
       return GLOBERROR("bad range", *posp, CURLE_URL_MALFORMAT);
 
@@ -230,9 +231,9 @@ static CURLcode glob_range(URLGlob *glob, char **patternp,
     pat->content.CharRange.ptr_c = pat->content.CharRange.min_c = min_c;
     pat->content.CharRange.max_c = max_c;
 
-    if(multiply(amount, (pat->content.CharRange.max_c -
+    if(multiply(amount, ((pat->content.CharRange.max_c -
                           pat->content.CharRange.min_c) /
-                         pat->content.CharRange.step + 1) )
+                         pat->content.CharRange.step + 1)))
       return GLOBERROR("range overflow", *posp, CURLE_URL_MALFORMAT);
   }
   else if(ISDIGIT(*pattern)) {
@@ -293,7 +294,9 @@ static CURLcode glob_range(URLGlob *glob, char **patternp,
     fail:
     *posp += (pattern - *patternp);
 
-    if(!endp || (min_n > max_n) || (step_n > (max_n - min_n)) || !step_n)
+    if(!endp || !step_n ||
+       (min_n == max_n && step_n != 1) ||
+       (min_n != max_n && (min_n > max_n || step_n > (max_n - min_n))))
       /* the pattern is not well-formed */
       return GLOBERROR("bad range", *posp, CURLE_URL_MALFORMAT);
 
@@ -303,9 +306,9 @@ static CURLcode glob_range(URLGlob *glob, char **patternp,
     pat->content.NumRange.max_n = max_n;
     pat->content.NumRange.step = step_n;
 
-    if(multiply(amount, (pat->content.NumRange.max_n -
-                         pat->content.NumRange.min_n) /
-                        pat->content.NumRange.step + 1) )
+    if(multiply(amount, ((pat->content.NumRange.max_n -
+                          pat->content.NumRange.min_n) /
+                         pat->content.NumRange.step + 1)))
       return GLOBERROR("range overflow", *posp, CURLE_URL_MALFORMAT);
   }
   else

@jay jay changed the title I recently upgraded to the newest curl and ran into a problem with globbing ranges. curl tool globbing range [1-1] doesn't work Feb 1, 2017
@bagder
Copy link
Member

bagder commented Feb 3, 2017

I made them not work (at some point when I went through and tightened up the range handling), because 1-1 isn't really range, it is just a single value and thus I think it is more likely to hint that there was a mistake made than that the user actually asks for a single value written in a clumsy way...

@jay
Copy link
Member

jay commented Feb 3, 2017

You may have a script that sets the range for a series of transfers, and maybe there's only one transfer to make. @rdsteed what's your use case?

@rdsteed
Copy link
Author

rdsteed commented Feb 4, 2017

@jay that is exactly the case. It is from a Windows batch file wrapping a curl command for downloading of podcast files. 1 to many years, 1 to many months, 1 to many days in one command. It also facilitated the #n notation in curl output for naming the downloads.

@Badger I found the [1-1] consistent with being able to use (a1..a1) notation in Excel spreadsheets to specify a single cell range or [1:2] to specify a single list item in Python. Of course, it doesn't make a whole lot of sense in these examples where constants are used, but it actually makes a lot of sense when the curl command is in a batch file and there is substitution of command arguments.

@bagder
Copy link
Member

bagder commented Feb 4, 2017

ok, so let's fix this and add a test or two to make sure it sticks

@rdsteed
Copy link
Author

rdsteed commented Feb 4, 2017

Thanks.
I'll just add that my expectation was that curl was interpreting the bracket notation to a for loop equivalent as follows:

[start-stop:step]
becomes
for (i = start; i <= stop; i+=step)

With this in mind
start > stop results in a zero trip loop
start=stop results in a one trip loop
step > (stop-start) results in a one trip loop.

I don't believe one trip loops should cause an error, but a warning in verbose mode could be helpful.

Finally, though I have a reading knowledge of C, I've never written in it. Otherwise, I would have contributed a pull request or a patch. Thanks for doing this.

@rdsteed rdsteed closed this as completed Feb 4, 2017
@rdsteed rdsteed reopened this Feb 4, 2017
@rdsteed
Copy link
Author

rdsteed commented Feb 4, 2017

Oops. Didn't mean to close. Clicked the wrong button. Still not used to this new touchpad.

@bagder
Copy link
Member

bagder commented Feb 7, 2017

@jay, will you bring this forward? You can probably make a test for it based on test87. If not, let me know and I can work on it.

jay added a commit to jay/curl that referenced this issue Feb 15, 2017
For example allow ranges like [1-1] and [a-a] etc.

Regression since 5ca96cb.

Bug: curl#1238
Reported-by: R. Dennis Steed
@jay
Copy link
Member

jay commented Feb 15, 2017

Ok, I added a test and made a branch for review. It is slightly different from my patch above because I had to make some adjustments to allow for [a-a:1].
https://github.com/curl/curl/compare/master...jay:fix_glob_range?expand=1

@bagder
Copy link
Member

bagder commented Feb 15, 2017

Looks good to me!

@jay
Copy link
Member

jay commented Feb 15, 2017

Thanks and thanks for your report @rdsteed, fix just landed in 7a9f574.

@jay jay closed this as completed Feb 15, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants