This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5772

hg: raise Abort on invalid path
ClosedPublic

Authored by indygreg on Jan 30 2019, 8:23 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG7f366dd3df1f: hg: raise Abort on invalid path
rHGc3b9988102a2: hg: raise Abort on invalid path
rHG2fe2f1ba596d: hg: raise Abort on invalid path
Summary

Currently, some os.path functions when opening repositories may
raise an uncaught TypeError or ValueError if the path is invalid.

Let's catch these exceptions and turn them into an Abort for
convenience.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Jan 30 2019, 8:23 PM
Closed by commit rHG2fe2f1ba596d: hg: raise Abort on invalid path (authored by indygreg). · Explain WhyJan 30 2019, 8:43 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jan 31 2019, 7:55 AM

+ try:
+ isfile = os.path.isfile(path)
+ # Python 2 raises TypeError, Python 3 ValueError.
+ except (TypeError, ValueError) as e:
+ raise error.Abort(_('invalid path %s: %s') % (
+ path, pycompat.bytestr(e)))

I don't know if the path is included in the exception message, but it's
probably safer to use stringutil.forcebytestr(e) to get around possible
UnicodeError.

martinvonz added a subscriber: martinvonz.Feb 2 2019, 12:38 AM

Currently, some os.path functions when opening repositories may
raise an uncaught TypeError or ValueError if the path is invalid.

What kind of "invalid"? If the path doesn't exist? Or only if the path variable is of the wrong type (not bytes? not unicode? neither?)?

yuja added a comment.Feb 2 2019, 1:37 AM
> Currently, some os.path functions when opening repositories may
>  raise an uncaught TypeError or ValueError if the path is invalid.
What kind of "invalid"? If the path doesn't exist? Or only if the path variable is of the wrong type (not bytes? not unicode? neither?)?

It's NUL byte in path string.

spectral updated this revision to Diff 15304.May 30 2019, 5:16 PM
spectral added a subscriber: spectral.May 30 2019, 5:21 PM

Oops. A typo caused me to update this diff, please ignore that.

Revision Contents
Changeset List

PathPackages
Mmercurial/hg.py (11 lines)
Mmercurial/ui.py (6 lines)
Mtests/test-remotefilelog-gc.t (6 lines)

Diff 13637

mercurial/hg.py

localrepo, localrepo,
lock, lock,
logcmdutil, logcmdutil,
logexchange, logexchange,
merge as mergemod, merge as mergemod,
narrowspec, narrowspec,
node, node,
phases, phases,
pycompat,
repository as repositorymod, repository as repositorymod,
scmutil, scmutil,
sshpeer, sshpeer,
statichttprepo, statichttprepo,
ui as uimod, ui as uimod,
unionrepo, unionrepo,
url, url,
util, util,
verify as verifymod, verify as verifymod,
vfs as vfsmod, vfs as vfsmod,
) )
release = lock.release release = lock.release
# shared features # shared features
sharedbookmarks = 'bookmarks' sharedbookmarks = 'bookmarks'
def _local(path): def _local(path):
path = util.expandpath(util.urllocalpath(path)) path = util.expandpath(util.urllocalpath(path))
return (os.path.isfile(path) and bundlerepo or localrepo)
try:
isfile = os.path.isfile(path)
# Python 2 raises TypeError, Python 3 ValueError.
except (TypeError, ValueError) as e:
raise error.Abort(_('invalid path %s: %s') % (
path, pycompat.bytestr(e)))
return isfile and bundlerepo or localrepo
def addbranchrevs(lrepo, other, branches, revs): def addbranchrevs(lrepo, other, branches, revs):
peer = other.peer() # a courtesy to callers using a localrepo for other peer = other.peer() # a courtesy to callers using a localrepo for other
hashbranch, branches = branches hashbranch, branches = branches
if not hashbranch and not branches: if not hashbranch and not branches:
x = revs or None x = revs or None
if revs: if revs:
y = revs[0] y = revs[0]

mercurial/ui.py

value = func(ui, self, suboptions[suboption]) value = func(ui, self, suboptions[suboption])
setattr(self, attr, value) setattr(self, attr, value)
def _isvalidlocalpath(self, path): def _isvalidlocalpath(self, path):
"""Returns True if the given path is a potentially valid repository. """Returns True if the given path is a potentially valid repository.
This is its own function so that extensions can change the definition of This is its own function so that extensions can change the definition of
'valid' in this case (like when pulling from a git repo into a hg 'valid' in this case (like when pulling from a git repo into a hg
one).""" one)."""
try:
return os.path.isdir(os.path.join(path, '.hg')) return os.path.isdir(os.path.join(path, '.hg'))
# Python 2 may return TypeError. Python 3, ValueError.
except (TypeError, ValueError):
return False
@property @property
def suboptions(self): def suboptions(self):
"""Return sub-options and their values for this path. """Return sub-options and their values for this path.
This is intended to be used for presentation purposes. This is intended to be used for presentation purposes.
""" """
d = {} d = {}

tests/test-remotefilelog-gc.t

$ echo " " > $CACHEDIR/repos $ echo " " > $CACHEDIR/repos
$ hg gc $ hg gc
warning: no valid repos in repofile warning: no valid repos in repofile
$ mv $CACHEDIR/repos.bak $CACHEDIR/repos $ mv $CACHEDIR/repos.bak $CACHEDIR/repos
# Test that warning is displayed when the repo path is malformed # Test that warning is displayed when the repo path is malformed
$ printf "asdas\0das" >> $CACHEDIR/repos $ printf "asdas\0das" >> $CACHEDIR/repos
$ hg gc 2>&1 | head -n2 $ hg gc
warning: malformed path: * (glob) abort: invalid path asdas\x00da: stat() argument 1 must be encoded string without null bytes, not str (esc)
Traceback (most recent call last): [255]