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

Comments

Projects
None yet
3 participants
@rdsteed

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 1, 2017

Member

Please fill out the template, it is not filled out

Member

jay commented Feb 1, 2017

Please fill out the template, it is not filled out

@jay jay added the cmdline tool label Feb 1, 2017

@rdsteed

This comment has been minimized.

Show comment
Hide comment
@rdsteed

rdsteed 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.

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 1, 2017

Member

[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
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 from I recently upgraded to the newest curl and ran into a problem with globbing ranges. to curl tool globbing range [1-1] doesn't work Feb 1, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 3, 2017

Member

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...

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 3, 2017

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@rdsteed

rdsteed 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.

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 4, 2017

Member

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

Member

bagder commented Feb 4, 2017

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

@rdsteed

This comment has been minimized.

Show comment
Hide comment
@rdsteed

rdsteed 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 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 Feb 4, 2017

@rdsteed rdsteed reopened this Feb 4, 2017

@rdsteed

This comment has been minimized.

Show comment
Hide comment
@rdsteed

rdsteed Feb 4, 2017

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

rdsteed commented Feb 4, 2017

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

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 7, 2017

Member

@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.

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

tool_urlglob: Allow a glob range with the same start and stop
For example allow ranges like [1-1] and [a-a] etc.

Regression since 5ca96cb.

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 15, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 15, 2017

Member

Looks good to me!

Member

bagder commented Feb 15, 2017

Looks good to me!

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 15, 2017

Member

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

Member

jay commented Feb 15, 2017

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

@jay jay closed this 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.