| Submitter | Matt Harbison |
|---|---|
| Date | Feb. 6, 2019, 2:04 a.m. |
| Message ID | <0e18c6ec895542394c0a.1549418640@Envy> |
| Download | mbox | patch |
| Permalink | /patch/38467/ |
| State | Superseded |
| Headers | show |
Comments
On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison <mharbison72@gmail.com> wrote: > # HG changeset patch > # User Matt Harbison <matt_harbison@yahoo.com> > # Date 1549417854 18000 > # Tue Feb 05 20:50:54 2019 -0500 > # Branch stable > # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b > # Parent 8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f > subrepo: avoid false unsafe path detection on Windows The fixes a last minute breakage in 4.9. It doesn't look like TortoiseHg or the MSI installer has been generated yet, but the Inno installer is already posted. Maybe this needs a 4.9.1?
On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison wrote: > # HG changeset patch > # User Matt Harbison <matt_harbison@yahoo.com> > # Date 1549417854 18000 > # Tue Feb 05 20:50:54 2019 -0500 > # Branch stable > # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b > # Parent 8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f > subrepo: avoid false unsafe path detection on Windows > > Subrepo paths are not normalized for the OS, so what was happening in the > subsequent root path check was: > > root -> $TESTTMP\issue1852a\sub/repo > util.expandpath(...) -> $TESTTMP\issue1852a\sub/repo > os.path.realpath(...) -> $TESTTMP\issue1852a\sub\repo Oops, my bad. > diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py > --- a/mercurial/subrepo.py > +++ b/mercurial/subrepo.py > @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo): > super(hgsubrepo, self).__init__(ctx, path) > self._state = state > r = ctx.repo() > - root = r.wjoin(path) > + root = os.path.realpath(r.wjoin(path)) Can we do r.wjoin(util.localpath(path)) instead? Even though symlinks and ".."s should be checked before, I want to avoid path resolution here for extra safety. What I'm not certain is whether realpath() does normalize long/short names and lower/upper case stuff. os.path.realpath() appears to call GetFullPathName() on Windows, and I guess it wouldn't do such normalization, but I'm not sure. https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfullpathnamea
On Wed, 06 Feb 2019 06:54:02 -0500, Yuya Nishihara <yuya@tcha.org> wrote: > On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison wrote: >> # HG changeset patch >> # User Matt Harbison <matt_harbison@yahoo.com> >> # Date 1549417854 18000 >> # Tue Feb 05 20:50:54 2019 -0500 >> # Branch stable >> # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b >> # Parent 8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f >> subrepo: avoid false unsafe path detection on Windows >> >> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py >> --- a/mercurial/subrepo.py >> +++ b/mercurial/subrepo.py >> @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo): >> super(hgsubrepo, self).__init__(ctx, path) >> self._state = state >> r = ctx.repo() >> - root = r.wjoin(path) >> + root = os.path.realpath(r.wjoin(path)) > > Can we do r.wjoin(util.localpath(path)) instead? Even though symlinks and > ".."s should be checked before, I want to avoid path resolution here for > extra safety. Early indications are yes. If the rest of the tests pass, I'll resend. I'm not real sure what the reason for avoiding path resolution is, so feel free to add commentary to the patch if you queue it. > What I'm not certain is whether realpath() does normalize long/short > names > and lower/upper case stuff. os.path.realpath() appears to call > GetFullPathName() on Windows, and I guess it wouldn't do such > normalization, > but I'm not sure. > > https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfullpathnamea It doesn't look like it changes either case: C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\PROGRAM FILES')" C:\PROGRAM FILES C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\PROGRA~1')" C:\PROGRA~1 C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\PROGRA~1\..\PROGRA~1')" C:\PROGRA~1 C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\PROgRA~1\..\PROGRa~1')" C:\PROGRa~1 C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\WINDOWS')" C:\WINDOWS C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\windows')" C:\windows I didn't have time to track this down, but I vaguely remember fixing some insane Windows path issue that somebody filed a bug about. It was something along the lines of: cd D:\path\to\repo_pool cd C:\Users\foo hg -R D:repo status ... and expecting that to expand to D:\path\to\repo_pool\repo. I don't remember if it was subrepo related, but I remember needing to use realpath() somewhere. There definitely aren't tests for it.
On Wed, 06 Feb 2019 23:16:43 -0500, Matt Harbison wrote: > On Wed, 06 Feb 2019 06:54:02 -0500, Yuya Nishihara <yuya@tcha.org> wrote: > > On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison wrote: > >> # HG changeset patch > >> # User Matt Harbison <matt_harbison@yahoo.com> > >> # Date 1549417854 18000 > >> # Tue Feb 05 20:50:54 2019 -0500 > >> # Branch stable > >> # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b > >> # Parent 8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f > >> subrepo: avoid false unsafe path detection on Windows > >> > >> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py > >> --- a/mercurial/subrepo.py > >> +++ b/mercurial/subrepo.py > >> @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo): > >> super(hgsubrepo, self).__init__(ctx, path) > >> self._state = state > >> r = ctx.repo() > >> - root = r.wjoin(path) > >> + root = os.path.realpath(r.wjoin(path)) > > > > Can we do r.wjoin(util.localpath(path)) instead? Even though symlinks and > > ".."s should be checked before, I want to avoid path resolution here for > > extra safety. > > Early indications are yes. If the rest of the tests pass, I'll resend. > I'm not real sure what the reason for avoiding path resolution is, so feel > free to add commentary to the patch if you queue it. This is a sanity check for bad subrepo paths, so I think it's better to not trust that the given path is valid.
Patch
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py --- a/mercurial/subrepo.py +++ b/mercurial/subrepo.py @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo): super(hgsubrepo, self).__init__(ctx, path) self._state = state r = ctx.repo() - root = r.wjoin(path) + root = os.path.realpath(r.wjoin(path)) create = allowcreate and not r.wvfs.exists('%s/.hg' % path) # repository constructor does expand variables in path, which is # unsafe since subrepo path might come from untrusted source.
# HG changeset patch # User Matt Harbison <matt_harbison@yahoo.com> # Date 1549417854 18000 # Tue Feb 05 20:50:54 2019 -0500 # Branch stable # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b # Parent 8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f subrepo: avoid false unsafe path detection on Windows Subrepo paths are not normalized for the OS, so what was happening in the subsequent root path check was: root -> $TESTTMP\issue1852a\sub/repo util.expandpath(...) -> $TESTTMP\issue1852a\sub/repo os.path.realpath(...) -> $TESTTMP\issue1852a\sub\repo