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

Closed
fstab opened this issue Nov 6, 2018 · 10 comments
Closed

path/filepath: always validate full Glob patterns #28614

fstab opened this issue Nov 6, 2018 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@fstab
Copy link

fstab 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
@ianlancetaylor ianlancetaylor changed the title Proposal: path/filepath: function to validate Glob patterns proposal: path/filepath: function to validate Glob patterns Nov 6, 2018
@ghost
Copy link

ghost 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
Copy link
Member

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

@andybons andybons added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Dec 5, 2018
@andybons andybons modified the milestones: Proposal, Go1.13 Dec 5, 2018
@rillig
Copy link
Contributor

rillig 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

@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
@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Backlog Dec 5, 2019
jmartin82 added a commit to jmartin82/go that referenced this issue Mar 23, 2020
The current implementation of the glob in Match(pattern, name) function
is evaluating the pattern syntax in a lazy way, this can lead to some mistakes,
due to a call with an invalid pattern can end up without match or
returning ErrBadPattern error depending on the input.

This change forces the pattern syntax validation, avoiding inconsistent error
return depending on the input, this change shouldn't break the compatibility
even without the correct error handling but can imply a performance penalty
when the input is empty.

Fixes golang#28614

Change-Id: I055a3b707b93f8ddda87551effb90f5e767a4117
GitHub-Last-Rev: 2099052
GitHub-Pull-Request: golang#33189
@gopherbot
Copy link

Change https://golang.org/cl/224900 mentions this issue: path: enforce glob pattern syntax validation on Match function

@gopherbot
Copy link

Change https://golang.org/cl/224997 mentions this issue: path: enforce glob pattern syntax validation on Match function

@gopherbot
Copy link

Change https://golang.org/cl/264397 mentions this issue: io/fs, path, path/filepath, testing/fstest: validate patterns in Match, Glob

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
9 participants