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

lib/osutil: Prevent infinite Glob recursion (fixes #3577) #3665

Closed
wants to merge 1 commit into from

Conversation

tpng
Copy link
Contributor

@tpng tpng commented Oct 12, 2016

This prevents filepath.Split(pattern) from returning the same dir value as pattern which cause infinite recursion on bad input.

Testing

In GUI, click "Add folder".
In folder path type: \?\C:\Users

@calmh
Copy link
Member

calmh commented Oct 12, 2016

@st-jenkins add to white list

@calmh
Copy link
Member

calmh commented Oct 12, 2016

Awesome. Could you add a test for this also, that runs on windows only?

@tpng
Copy link
Contributor Author

tpng commented Oct 12, 2016

Added test for Windows that will panic on unfixed code.

@AudriusButkevicius
Copy link
Member

You should check the runtime.GOOS or add +build windows to the header, as this will most likely consistently fail on non-Windows

@tpng
Copy link
Contributor Author

tpng commented Oct 12, 2016

Oh test ignore the _windows_ in the filename?

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented Oct 12, 2016

I actually didn't notice that and I don't know, interesting question.

@tpng
Copy link
Contributor Author

tpng commented Oct 12, 2016

I got the same question when writing the test too, so I took a look at how they write the platform specific test in Go src, and go test seems to follow the same rule as normal code on filename. (e.g. skipping _unix_ test files on Windows)

edit: I see they had a build flag at top too, I will add it for safe then.

This prevent filepath.Split(pattern) from returning the same `dir` value as `pattern` which cause infinite recursion on bad input.
@AudriusButkevicius
Copy link
Member

In which was I withdraw my comment.

@canton7
Copy link
Member

canton7 commented Oct 12, 2016

(BTW, the go fmt check is failing - run go fmt on your code).

@tpng
Copy link
Contributor Author

tpng commented Oct 12, 2016

The fmt fail is on cmd/syncthing/traceback.go, do you want me to go fmt it and add to the changeset or left it as is?

@canton7
Copy link
Member

canton7 commented Oct 12, 2016

Ah? Ignore me in that case.

@calmh
Copy link
Member

calmh commented Oct 12, 2016

@st-jenkins retest this please

@calmh
Copy link
Member

calmh commented Oct 12, 2016

@tpng Thanks, gofmt fixed (my bad), test confirmed to run on Windows and fails without the patch!

Now just to get Jenkins to play ball...

@calmh
Copy link
Member

calmh commented Oct 12, 2016

@st-jenkins retest this please

@calmh
Copy link
Member

calmh commented Oct 12, 2016

@st-review merge it

lib/osutil: Prevent infinite Glob recursion (fixes #3577)

Skip-check: authors

@st-review
Copy link

👌 Merged as 05c37e5. Thanks, @tpng!

@st-review st-review closed this Oct 12, 2016
st-review pushed a commit that referenced this pull request Oct 12, 2016
Skip-check: authors

GitHub-Pull-Request: #3665
@calmh
Copy link
Member

calmh commented Oct 12, 2016

(there is nothing wrong with the authorship, it's just Jenkins has some issues atm)

@canton7
Copy link
Member

canton7 commented Oct 12, 2016

👍 thanks @tpng!

@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 15, 2017
@syncthing syncthing locked and limited conversation to collaborators Nov 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants