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

singleurl: fix query_is_modified #323

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

emanuele6
Copy link
Collaborator

query_is_modified is global and does not get reset at every call of singleurl() so, if a query is modified for a URL, the query for all the following URLs is automatically considered modified.

Additionally --trim and --replace were setting query_is_modified to true unconditionally as long as a --trim or --replace option is passed to trurl, even if no changes were made.
Which made it effectively just a convoluted way to check if any of --trim, --replace, --append query, --sort-query was passed.

Since singleurl() was made recursive to implement --iterate, I could not just add query_is_modified = false at the start of singleurl() to fix the problem, so I have refactored the code to not need a global.

return; /* out of memory, bail out */
return query_is_modified; /* out of memory, bail out */
Copy link
Collaborator Author

@emanuele6 emanuele6 Aug 16, 2024

Choose a reason for hiding this comment

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

Wouldn't continue make more sense if it does not just fail? Or wouldn't it make more sense to just error instead of possibly outputting a wrong result?

Copy link
Member

Choose a reason for hiding this comment

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

Either this should exit immediately with an error, or we need to make the function return an error code so that the parent caller can spot the error. We should not let it continue and output wrong.

Copy link
Collaborator Author

@emanuele6 emanuele6 Aug 16, 2024

Choose a reason for hiding this comment

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

Currently this return is just ignored, as if this and the following --trims didn't match anything

query_is_modified is global and does not get reset at every call of
singleurl() so, if a query is modified for a URL, the query for all the
following URLs is automatically considered modified.

Additionally --trim and --replace were setting query_is_modified to true
unconditionally as long as a --trim or --replace option is passed to
trurl, even if no changes were made.
Which made it effectively just a convoluted way to check if any of
--trim, --replace, --append query, --sort-query was passed.

Since singleurl() was made recursive to implement --iterate, I could not
just add  query_is_modified = false  at the start of singleurl() to fix
the problem, so I have refactored the code to not need a global.
@bagder
Copy link
Member

bagder commented Aug 16, 2024

It would be most helpful if you also added a test case for the scenario this fixes.

@emanuele6
Copy link
Collaborator Author

@bagder This doesn't fix behaviour; it fixes query_is_modified being set to true unnecessarily, making trurl do extra work unnecessarily.

@bagder
Copy link
Member

bagder commented Aug 16, 2024

ah thanks, I missed that nuance

@emanuele6
Copy link
Collaborator Author

For example:
ltrace trurl --replace 'a=x' 'https://example.com/?'{a,b}'=z' without the patch:

 strchr("b=z", '&')                                                                                       = nil
 strlen("b=z")                                                                                            = 3
 malloc(16)                                                                                               = 0x559e1dcfade0
 malloc(4)                                                                                                = 0x559e1dcfa240
 memcpy(0x559e1dcfa240, "b=z", 3)                                                                         = 0x559e1dcfa240
 memchr("b=z", '=', 3)                                                                                    = 0x559e1dcf9fd1
 curl_easy_unescape(0, 0x559e1dcf9fd0, 1, 0x7fff2da4b144)                                                 = 0x559e1dd00390
 curl_easy_unescape(0, 0x559e1dcf9fd2, 1, 0x7fff2da4b140)                                                 = 0x559e1dcfa010
 malloc(3)                                                                                                = 0x559e1dd00370
 memcpy(0x559e1dd00370, "b", 1)                                                                           = 0x559e1dd00370
 memcpy(0x559e1dd00372, "z", 1)                                                                           = 0x559e1dd00372
 curl_free(0x559e1dcfa010, 0x559e1dcfa010, 1, 122)                                                        = 1
 curl_free(0x559e1dd00390, 0x559e1dcfa000, 0x559e1dcfa, 33)                                               = 2
 malloc(16)                                                                                               = 0x559e1dd00390
 free(0x559e1dd00390)                                                                                     = <void>
 free(0x559e1dcfade0)                                                                                     = <void>
 curl_free(0x559e1dcf9fd0, 0x559e1dcfadd0, 0x559b4431df6a, 33)                                            = 4
 strchr("a=x", '=')                                                                                       = "=x"
 strlen("x")                                                                                              = 1
 strncmp("b=z", "a=x", 1)                                                                                 = 1
+curl_maprintf(0x559e1d917138, 0x559e1d91718a, 0x559e1d91718a, 0x559e1dcfa240)                            = 0x559e1dcfb7b0
+curl_free(0, 0x7fff2da496b7, 0, 122)                                                                     = 0x7fe38bbfb5b8
+curl_url_set(0x559e1dd01180, 8, 0x559e1dcfb7b0, 0)                                                       = 0
+curl_free(0x559e1dcfb7b0, 0x559e1dcfc040, 0x559e1dcfc, 0x559e1dcf9fd0)                                   = 2
 curl_url_get(0x559e1dd01180, 0, 0x7fff2da4b230, 74)                                                      = 0
 puts("https://example.com/?b=z")                                                                         = 25

ltrace ./trurl --replace 'a=x' 'https://example.com/?'{a,b}'=z' with the patch:

[...]
 strchr("b=z", '&')                                                                                       = nil
 strlen("b=z")                                                                                            = 3
 malloc(16)                                                                                               = 0x557cb7b49de0
 malloc(4)                                                                                                = 0x557cb7b49240
 memcpy(0x557cb7b49240, "b=z", 3)                                                                         = 0x557cb7b49240
 memchr("b=z", '=', 3)                                                                                    = 0x557cb7b48fd1
 curl_easy_unescape(0, 0x557cb7b48fd0, 1, 0x7ffdb4e9bac0)                                                 = 0x557cb7b4f390
 curl_easy_unescape(0, 0x557cb7b48fd2, 1, 0x7ffdb4e9babc)                                                 = 0x557cb7b49010
 malloc(3)                                                                                                = 0x557cb7b4f370
 memcpy(0x557cb7b4f370, "b", 1)                                                                           = 0x557cb7b4f370
 memcpy(0x557cb7b4f372, "z", 1)                                                                           = 0x557cb7b4f372
 curl_free(0x557cb7b49010, 0x557cb7b49010, 1, 0x557cb7b4f37a)                                             = 1
 curl_free(0x557cb7b4f390, 0x557cb7b49000, 0x557cb7b49, 33)                                               = 2
 malloc(16)                                                                                               = 0x557cb7b4f390
 free(0x557cb7b4f390)                                                                                     = <void>
 free(0x557cb7b49de0)                                                                                     = <void>
 curl_free(0x557cb7b48fd0, 0x557cb7b49dd0, 0x5579e07f88d9, 33)                                            = 4
 strchr("a=x", '=')                                                                                       = "=x"
 strlen("x")                                                                                              = 1
 strncmp("b=z", "a=x", 1)                                                                                 = 1
-curl_maprintf
-curl_free
-curl_url_set
-curl_free
 curl_url_get(0x557cb7b50180, 0, 0x7ffdb4e9bc50, 74)                                                      = 0
 puts("https://example.com/?b=z")                                                                         = 25
[...]

@bagder bagder merged commit 40c0a73 into curl:master Aug 16, 2024
10 checks passed
@bagder
Copy link
Member

bagder commented Aug 16, 2024

Thanks!

@emanuele6 emanuele6 deleted the fixqueryismod branch August 16, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants