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

Add warning for ignored data after quoted form parameter #7394

Closed
wants to merge 2 commits into from

Conversation

@blyxxyz
Copy link
Contributor

@blyxxyz blyxxyz commented Jul 14, 2021

In an argument like -F 'x=@/etc/hostname;filename="foo"abc' the abc is ignored. This adds a warning if the ignored data isn't all whitespace.

I don't know if this warrants a test, I couldn't find many tests for warnings.

@@ -381,8 +384,18 @@ static char *get_param_word(char **str, char **end_pos, char endchar)
while(ptr < *end_pos);
*end_pos = ptr2;
}
while(*ptr && *ptr != ';' && *ptr != endchar)
++ptr;
Copy link
Member

@danielgustafsson danielgustafsson Jul 14, 2021

Why the extra increment?

Copy link
Contributor Author

@blyxxyz blyxxyz Jul 14, 2021

To advance past the closing ", which would otherwise trigger the ISSPACE check.

Copy link
Member

@jay jay Jul 19, 2021

Why the extra increment?

I'm also curious about this. When the string is unescaped then can ptr be advanced past *end_pos in the escaped loop (passed the pointer to the end "). Also could *ptr == endchar. In either case should we be incrementing it

Copy link
Contributor Author

@blyxxyz blyxxyz Jul 19, 2021

I think ptr == end_pos is guaranteed, since the final " wasn't escaped and so can't be skipped past. (If it was escaped then it would be considered missing and we wouldn't end up here.)

But to make it more robust, maybe I could do

Suggested change
++ptr;
ptr = *end_pos + 1;

(EDIT: this doesn't work because end_pos is modified right above)

or

Suggested change
++ptr;
if(*ptr == '"')
++ptr;

or down below:

          if(!ISSPACE(*ptr) && *ptr != '"')

(though that last one would prevent some reasonable warnings)

Copy link
Member

@jay jay Jul 26, 2021

I think ptr == end_pos is guaranteed, since the final " wasn't escaped and so can't be skipped past. (If it was escaped then it would be considered missing and we wouldn't end up here.)

Looks like you're right but AFAICT it's still possible that a double quote could be used as endchar.

Suggested change
++ptr;
if(*ptr != endchar)
++ptr;

Copy link
Contributor Author

@blyxxyz blyxxyz Aug 1, 2021

In practice endchar is always either \0 or , so it shouldn't matter and changing it isn't a compatibility hazard.

Even if endchar were " I don't think you'd want the closing quote to do double duty as a separator, so advancing past it unconditionally makes more sense than the old behavior.

(The purpose of endchar is to separate files in an argument like -Fx=@foo.txt;filename="foo",bar.txt;filename="bar". If it's \0 it's effectively ignored and it's only , for the =@ case.)

Copy link
Member

@jay jay Aug 2, 2021

A double quote may not make sense as the endchar currently but if it could happen I'd rather have the same behavior not to advance ptr if *ptr is endchar.

Copy link
Contributor Author

@blyxxyz blyxxyz Aug 2, 2021

Why? The new behavior IMO makes more sense.

The intent of the loop is to move past the end of the value to a delimiter that's after/between fields. If in -Fx=@foo.txt;filename="foo",bar.txt;filename=bar you replaced the , by a " and parsed it with an endchar of " it'd work with the new behavior but raise a warning with the old behavior. I think you'd want each " character to be used as either a quote or an endchar, but not both at the same time.

Copy link
Member

@jay jay Aug 3, 2021

The loops in the function will not advance ptr when *ptr == endchar, so I prefer it for consistency. I'm for retaining the existing behavior even if what we are discussing is likely improbable.

Copy link
Contributor Author

@blyxxyz blyxxyz Aug 3, 2021

Isn't it currently impossible rather than improbable? It's a static function, only called by another static function which is only ever called with a char literal. endchar is also never assigned to or referenced.

If you don't think that's enough I'll defer to your judgment.

*ptr = '\0';
warnf(config->global, "Ignoring trailing data: %s\n", trail_start);
*ptr = oldchr;
}
Copy link
Member

@danielgustafsson danielgustafsson Jul 14, 2021

I wonder if there is enough value in printing the actual trailing data, or if we should simplify and just warn that there was data at all?

Copy link
Contributor Author

@blyxxyz blyxxyz Jul 14, 2021

I figured that printing the data can be a lot clearer than explaining the abstract rule, particularly if you have a longer command and you'd have to figure out which part it's coming from.

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Jul 14, 2021

In an argument like `-F 'x=@/etc/hostname;filename="foo"abc'` the
`abc` is ignored. This adds a warning if the ignored data isn't all
whitespace.
@blyxxyz blyxxyz force-pushed the warn-param-word-trail branch from ee38b39 to a453254 Jul 14, 2021
@blyxxyz
Copy link
Contributor Author

@blyxxyz blyxxyz commented Jul 14, 2021

Hmm, I see what you mean. I've changed it to a fixed message, hopefully that's clear enough.

@jay
Copy link
Member

@jay jay commented Jul 16, 2021

++ptr;
}
if(trailing_data)
warnf(config->global, "Trailing data after quoted file parameter\n");
Copy link
Contributor

@monnerat monnerat Jul 16, 2021

You mean "Trailing data after quoted form parameter\n" ?

Copy link
Contributor Author

@blyxxyz blyxxyz Jul 16, 2021

Yep, fixed.

@monnerat
Copy link
Contributor

@monnerat monnerat commented Jul 16, 2021

@jay At first glance, this seems ok. I've no objection fo this change.

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Jul 18, 2021

I would argue that this is new functionality and will mark it for the next feature window.

@bagder
Copy link
Member

@bagder bagder commented Aug 17, 2021

Thanks!

@blyxxyz blyxxyz deleted the warn-param-word-trail branch Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants