Add warning for ignored data after quoted form parameter #7394

blyxxyz · 2021-07-14T22:08:33Z

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.

danielgustafsson · 2021-07-14T22:12:43Z

src/tool_formparse.c

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


Why the extra increment?

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

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

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)

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;

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

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.

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.

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.

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.

danielgustafsson · 2021-07-14T22:14:09Z

src/tool_formparse.c

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


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?

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 · 2021-07-14T22:30:35Z

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.

There is that. Operating on arbitrary user input which was never correct in the first place does carry a security risk though. That being said, I don't see an attack vector angle here but it's treading onto territory which can be minefields for future bugs.

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 · 2021-07-14T22:37:44Z

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

jay · 2021-07-16T18:28:21Z

@monnerat

monnerat · 2021-07-16T22:31:21Z

src/tool_formparse.c

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


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

Yep, fixed.

monnerat · 2021-07-16T22:38:30Z

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

danielgustafsson · 2021-07-18T20:27:48Z

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

bagder · 2021-08-17T08:21:19Z

Thanks!

danielgustafsson reviewed Jul 14, 2021

View reviewed changes

Add warning for ignored data after quoted form parameter

a453254

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 force-pushed the warn-param-word-trail branch from ee38b39 to a453254 Compare July 14, 2021 22:37

jay added the cmdline tool label Jul 16, 2021

monnerat reviewed Jul 16, 2021

View reviewed changes

fixup! Add warning for ignored data after quoted form parameter

9d2b9e8

danielgustafsson added the feature-window A merge of this requires an open feature window label Jul 18, 2021

bagder removed the feature-window A merge of this requires an open feature window label Aug 17, 2021

bagder closed this in 50ddc14 Aug 17, 2021

blyxxyz deleted the warn-param-word-trail branch September 16, 2021 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning for ignored data after quoted form parameter #7394

Add warning for ignored data after quoted form parameter #7394

blyxxyz commented Jul 14, 2021

danielgustafsson Jul 14, 2021

blyxxyz Jul 14, 2021

jay Jul 19, 2021

blyxxyz Jul 19, 2021 •

edited

jay Jul 26, 2021

blyxxyz Aug 1, 2021

jay Aug 2, 2021

blyxxyz Aug 2, 2021

jay Aug 3, 2021

blyxxyz Aug 3, 2021

danielgustafsson Jul 14, 2021

blyxxyz Jul 14, 2021

danielgustafsson commented Jul 14, 2021 via email

blyxxyz commented Jul 14, 2021

jay commented Jul 16, 2021

monnerat Jul 16, 2021

blyxxyz Jul 16, 2021

monnerat commented Jul 16, 2021

danielgustafsson commented Jul 18, 2021

bagder commented Aug 17, 2021

Add warning for ignored data after quoted form parameter #7394

Add warning for ignored data after quoted form parameter #7394

Conversation

blyxxyz commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxxyz Jul 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielgustafsson commented Jul 14, 2021 via email

blyxxyz commented Jul 14, 2021

jay commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monnerat commented Jul 16, 2021

danielgustafsson commented Jul 18, 2021

bagder commented Aug 17, 2021

blyxxyz Jul 19, 2021 •

edited