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

path/filepath: always validate full Glob patterns #28614

Open
fstab opened this issue Nov 6, 2018 · 7 comments · May be fixed by #33189

Comments

@fstab
Copy link

commented Nov 6, 2018

The functions filepath.Match(pattern, name) and path.Match(pattern, name) return ErrBadPattern if the pattern is invalid, but due to lazy evaluation the error may or may not occur depending on what you match.

Example:

_, err := filepath.Match("a[", "a") // err is nil
_, err := filepath.Match("a[", "abc") // err is "syntax error in pattern"

As patterns are often user input, it would be handy to be able to validate the pattern. If the validation is successful, it should be guaranteed that filepath.Match() will not return an error.

I created a PR for this here and was asked to open a proposal issue first.

Other programming languages seem to either not validate the pattern at all (like Python's glob.glob('a[')), or always validate the pattern (like Java's FileSystems.getDefault().getPathMatcher("glob: a["))). I couldn't find an example where validation depends on what name is matched.

@tklauser tklauser changed the title [Proposal] Function to validate Glob patterns Proposal: path/filepath: function to validate Glob patterns Nov 6, 2018
@gopherbot gopherbot added this to the Proposal milestone Nov 6, 2018
@gopherbot gopherbot added the Proposal label Nov 6, 2018
@ianlancetaylor ianlancetaylor changed the title Proposal: path/filepath: function to validate Glob patterns proposal: path/filepath: function to validate Glob patterns Nov 6, 2018
@opennota

This comment has been minimized.

Copy link

commented Nov 7, 2018

An alternative: make Match evaluate pattern non-lazily. That way there would be no risk that the implementations will diverge and Match will return an error for pattern which passes IsPatternValid.

@andybons andybons changed the title proposal: path/filepath: function to validate Glob patterns path/filepath: always validate full Glob patterns Dec 5, 2018
@andybons

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

Per discussion with @golang/proposal-review, we believe that the Match function should evaluate non-lazily (@opennota's suggestion) so that one can use Match to validate a Glob without adding a new function.

@rillig

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@andybons I certainly like that the current implementation is as efficient as possible. I just miss the option to validate a pattern. Maybe calling path.Match(pattern, "") could be a special case that evaluates the complete pattern. This string is unlikely to be used in normal situations, therefore it shouldn't hurt much when the whole pattern is analyzed.

Alternatively there could be path.Compile(pattern) that just compiles a path expression. This would also allow for common special cases to be matched much faster (prefix, suffix), like I did it in a project of mine for the subset of path patterns that I really needed.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@jmartin82

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Hi, I've been checking this issue and I would like to work on it if no one else is doing it right now as my first contribution.

I see two possible implementations using the same functions.

  1. Adding some extra checks to the current implementation, keeping the laziness but ensuring a consistent repose in case of an invalid pattern (almost no overhead over the current implementation).

  2. Adding a guard clause to check the whole pattern validity straight away (with a small overhead over the current implementation)

As per your previous conversation, you seem more inclined to the second option, right?

Thanks,

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

We want to make sure that implementation is simple, efficient, and always detects an invalid pattern. I would tend to think that the second option you list is likely to be the better approach, but I think we're open to any implementation that meets those goals as best as possible. Thanks.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 19, 2019

Change https://golang.org/cl/186937 mentions this issue: path: This change enforces the pattern syntax evaluation before the matching process on Match function FIX #28614

@jmartin82

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

@ianlancetaylor I've created a PR (https://go-review.googlesource.com/c/go/+/186937) with a possible implementation for path.Match(pattern, name) function (it's a mix between option 1 and 2 😄 ) let me know what do you think about, before applying the same solution to the filepath.Match

Thanks,

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@smasher164 smasher164 modified the milestones: Backlog, Go1.14 Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.