Patchwork D7411: dirs: resolve fuzzer OOM situation by disallowing deep directory hierarchies

login
register
mail settings
Submitter phabricator
Date Nov. 14, 2019, 9:22 p.m.
Message ID <differential-rev-PHID-DREV-t7ikglxfjme6pz6pzltg-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43220/
State Superseded
Headers show

Comments

phabricator - Nov. 14, 2019, 9:22 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  It seems like 2048 directories ought to be enough for any reasonable
  use of Mercurial?
  
  .. bc:
  
    Mercurial will now defend against OOMs by refusing to operate on
    paths with 2048 or more components. This means that _extremely_
    deep path hierarchies will be rejected, but we anticipate nobody
    is using hierarchies this deep.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7411

AFFECTED FILES
  mercurial/cext/dirs.c

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 15, 2019, 3:04 a.m.
indygreg added a comment.


  I support this approach. But I'd feel better if we captured the performance implications. Especially since there is a comment just below talking about how important the loop is for performance.
  
  Perhaps we should add this check to the loop below by checking how m

INLINE COMMENTS

> dirs.c:66
> +	for (i = 0; i < pos; ++i) {
> +		if (cpath[i] == '/') {
> +			++num_slashes;

What code calls this function? Do we have any good perf numbers for introducing this loop?

I ask because the diffing code is surprisingly impacted by the the "find newlines" stage. Using an implementation that the compiler can expand to SSE/AVX instructions is substantially faster. FWIW glibc and other C implementations have assembly versions of `strchr()` and `memchr()`, which could be substantially faster if the compiler isn't smart enough to detect the "count occurrences of chars" pattern.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7411/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7411

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Nov. 15, 2019, 3:26 a.m.
durin42 added inline comments.
durin42 marked an inline comment as done.

INLINE COMMENTS

> indygreg wrote in dirs.c:66
> What code calls this function? Do we have any good perf numbers for introducing this loop?
> 
> I ask because the diffing code is surprisingly impacted by the the "find newlines" stage. Using an implementation that the compiler can expand to SSE/AVX instructions is substantially faster. FWIW glibc and other C implementations have assembly versions of `strchr()` and `memchr()`, which could be substantially faster if the compiler isn't smart enough to detect the "count occurrences of chars" pattern.

I'd be happy to use memchr()  but it occurred to me as I was failing to use memchr() effectively that we already find slashes in a loop here, and there's no risk of on-disk corruption so we can just count slashes as we populate the dict and stop. It's not nearly as fast in the fuzzer, but it does pass for the specific input that we're stuck on.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7411/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7411

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Nov. 15, 2019, 4:04 a.m.
This revision is now accepted and ready to land.
indygreg added a comment.
indygreg accepted this revision.


  This is much better. Thanks.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7411/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7411

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel

Patch

diff --git a/mercurial/cext/dirs.c b/mercurial/cext/dirs.c
--- a/mercurial/cext/dirs.c
+++ b/mercurial/cext/dirs.c
@@ -48,12 +48,30 @@ 
 	return pos;
 }
 
+/* Mercurial will fail to run on directory hierarchies deeper than
+ * this constant, so we should try and keep this constant as big as
+ * possible.
+ */
+#define MAX_DIRS_DEPTH 2048
+
 static int _addpath(PyObject *dirs, PyObject *path)
 {
 	const char *cpath = PyBytes_AS_STRING(path);
 	Py_ssize_t pos = PyBytes_GET_SIZE(path);
 	PyObject *key = NULL;
 	int ret = -1;
+	size_t num_slashes = 0, i;
+
+	for (i = 0; i < pos; ++i) {
+		if (cpath[i] == '/') {
+			++num_slashes;
+		}
+	}
+	if (num_slashes > MAX_DIRS_DEPTH) {
+		PyErr_SetString(PyExc_ValueError,
+		                "Directory hierarchy too deep.");
+		goto bail;
+	}
 
 	/* This loop is super critical for performance. That's why we inline
 	 * access to Python structs instead of going through a supported API.